Skip to content

Conversation

@rootkea
Copy link
Contributor

@rootkea rootkea commented Sep 10, 2021

Found by: scan-build

@masatake
Copy link
Member

Thank you.
Could you make a pull request only for 9e65c42?
In that case, could you use 'Tex: ' as prefix the commit header ?

Tex: add missing break statement

I would like to merge 9e65c42 first.

@rootkea
Copy link
Contributor Author

rootkea commented Sep 10, 2021

I just opened #3156 fixing missing break.

@rootkea rootkea marked this pull request as draft September 10, 2021 15:13
@masatake
Copy link
Member

masatake commented Sep 10, 2021

Could you make a pull request only for 27ec7c8 and send it to https://github.com/universal-ctags/libreadtags/pulls ?
ctags just uses the library developed at the repository.

@rootkea rootkea force-pushed the small-fixes branch 2 times, most recently from 23a6f97 to 6124bdf Compare September 10, 2021 17:51
@rootkea
Copy link
Contributor Author

rootkea commented Sep 10, 2021

Opened universal-ctags/libreadtags#34 for NULL check on file before accessing file->err

@rootkea rootkea marked this pull request as ready for review September 10, 2021 18:42
@masatake
Copy link
Member

I will cherry-pick the commits from this pull request incrementally.

@rootkea rootkea marked this pull request as draft September 11, 2021 10:37
@rootkea
Copy link
Contributor Author

rootkea commented Sep 11, 2021

I will cherry-pick the commits from this pull request incrementally.

Or if you want me to open separate PRs for each commit then I can also do that. :)

@rootkea
Copy link
Contributor Author

rootkea commented Sep 11, 2021

I have a build question:

Why don't I see #define DEBUG 1 in config.h even when I supply --enable-debugging to ./configure?

Looks like we're missing AC_DEFINE(DEBUG) in configure.ac?

@masatake
Copy link
Member

Why don't I see #define DEBUG 1 in config.h even when I supply --enable-debugging to ./configure?
Looks like we're missing AC_DEFINE(DEBUG) in configure.ac?

I don't know or I cannot remember.
Maybe there is a historical reason.

Universal Ctags (u-ctags) was derived from Exuberant Ctags (e-ctags).

e-ctags has its own debug infrastructure.
When I started working on u-ctags, I didn't know and study the debug-infra.
The debug infra was related to C language parser of e-ctags.
u-ctags introduced new C/C++ parsers. So I didn't find a strong reason to study the debug-infra.

On the other hand, u-ctags introduced a new debug-infra called "tracing".
It is a spin-off of the new C/C++ parsers.
./configure --enable-debuggin enables the tracing feature.

And now we are here. I often want to revise the debug-infra. However, I didn't.

@rootkea rootkea marked this pull request as ready for review September 15, 2021 20:26
@rootkea
Copy link
Contributor Author

rootkea commented Sep 15, 2021

@masatake Thanks for the reply!

Why don't I see #define DEBUG 1 in config.h even when I supply --enable-debugging to ./configure?

Answering my own question: We pass -DDEBUG instead of defining DEBUG in config.h for --enable-debugging.

@rootkea
Copy link
Contributor Author

rootkea commented Sep 15, 2021

I'll look into tests failures tomorrow.

@masatake
Copy link
Member

The failure is reproduced even on my note pc (make tmain).
After removing main: Refactor defaultDoesContainAnyChar() e08c926 from the branch, the failure is gone.

Could you remove the pull request from the this pull request?
So I can merge the "good" changes first.

Then make a separate pull request for main: Refactor defaultDoesContainAnyChar() e08c926 ?
I will also inspect the reason of the failure. I guess your change exposes a bug.

@masatake
Copy link
Member

  • main: Refactor defaultDoesContainAnyChar() e08c926

I have not reviewed yet because it make the test on the CD
enbvironment failure. I will take time to inspect the failure
because I think the change exposures pre-existing bug.

  • Remove redundant code 752d982

    I wonder how should I deal with this.
    The same/similar change is already proposed in Remove misc dead assignments #2280 and parser: Remove dead assignments #2278.
    I have not taken an action for them because I would like to keep
    dead assignments to make code understandable when extending parsers.

    It is not easy for me to explain why keeping dead assignments helps
    people understanding code. So I didn't take action.

    I wonder how I should do wit this change, Remove misc dead assignments #2280, and parser: Remove dead assignments #2278.

  • main: Allocate the exact amount of memory we need edf5194

    This is obvious improvement. I will cherry-pick this.

  • main: Use safer vsnprintf() instead of vsprintf() 5d1fa0f

    The size is already ensured by calling mem_try_ensure_space in line 842.
    So just using vsprintf is enough.
    I will not merge this change.

  • buildsys: Fix check for perl 80c50eb

    Using AC_CHECK_PROGS is bug?
    I don't think so. I will not merge this change.

masatake pushed a commit to masatake/ctags that referenced this pull request Sep 16, 2021
@rootkea
Copy link
Contributor Author

rootkea commented Sep 16, 2021

Using AC_CHECK_PROGS is bug?

Check the AC_CHECK_PROGS syntax.

Currently, if you have perl on your system the configure.ac fails to detect it.

@masatake
Copy link
Member

[yamato@dev64]~/var/ctags-github% which perl
which perl
/usr/bin/perl
[yamato@dev64]~/var/ctags-github% ./configure 2>&1 | grep perl
./configure 2>&1 | grep perl
checking for perl... no

You are correct. I will cherry pick 80c50eb, too. Thank you.

masatake pushed a commit to masatake/ctags that referenced this pull request Sep 16, 2021
@rootkea rootkea marked this pull request as draft September 16, 2021 09:56
@masatake
Copy link
Member

edf5194 and 80c50eb are merged.


Assert (tag);
Assert (index == NO_PARSER_FIELD || ((unsigned int)index) < tag->usedParserFields);
Assert (index == NO_PARSER_FIELD || (index >= 0 && ((unsigned int)index) < tag->usedParserFields));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line makes sense.

doesContainAnyChar = defaultDoesContainAnyChar;
}
if (index == NO_PARSER_FIELD)
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change doesn't consider the combination of index == NO_PARSER_FIELD and doesContainAnyChar != NULL.

@masatake
Copy link
Member

Acceptable changes are already merged to the main tree.
Thank you. So I will close this one in this time.

@masatake masatake closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants