Skip to content

Conversation

cohomology
Copy link
Contributor

@cohomology cohomology commented Jun 3, 2020

This PR does the following three changes to the C++ runtime of Antlr 4:

  1. It is considered an antipattern in C++ to write auto foo = some pointer type. Instead it should be preferred to write auto * foo = some pointer type. The same holds true for const auto. This commit changes this in all places in antlr4. This makes some static linters happy, which check this.

  2. There is exactly one place where thread_local is used in antlr4. As this is not needed at all (the constructor of std::wstring_convert is not expensive) this superfluous thread_local is removed. This makes antlr4 buildable with xlclang compiler on AIX.

  3. In CppUtils.cpp there is a true (non-ASCII) unicode character used in a char literal. This is replaced by the corresponding unicode character escape sequence. This makes some static linters happy, which check this.

I signed the contributors.txt.

Regards,
Cohomology

@cohomology cohomology changed the title Improve coding style of auto usages [C++ runtime]: Improve coding style of auto usages Jun 3, 2020
@cohomology
Copy link
Contributor Author

Timeout in travis ... I will try to do a dummy commit.

Copy link
Member

@mike-lischke mike-lischke left a comment

Choose a reason for hiding this comment

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

Everything else looks good!

Comment on lines 25 to 24
thread_local UTF32Converter converter;
UTF32Converter converter;
Copy link
Member

Choose a reason for hiding this comment

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

The thread_local storage is there for a reason. When you look back in the history of this file you will see that the code was once like you changed it, which lead to crashes. So the thread_local change must be rolled back to make this patch acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Mike,

there is no way that removing of thread_local can lead do crashes. I guess it crashed, when "static" was used. The relevant CL was "bda96a0081c06126b55b1569cc1586ffd3cb3ce8", saying "getText performance improvement by making UTF32Converter thread_local instead of local.", so the author claims a performance improvement by using "thread_local".

Okay, I will revert it then ... although thread_local is a heavy weapon, not supported on some platforms, i.e. AIX xlcclang compiler on AIX 7.2, which otherwise support C++14. I was trying to compile Antlr 4 there ;-).

Best regards,
Kilian.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, you are right it was static. But you should not forget that with that local var the utf converter (which is pretty heavy) is instantiated for every getText() call, which is often used for predicates, slowing down the parser significantly. So yes, please revert that change. Thanks.

@mike-lischke
Copy link
Member

@cohomology Please merge with the latest master to fix the conflict in contributors.txt.

@parrt Once the conflict is gone this is ready to be merged.

@parrt parrt merged commit aa5a0d2 into antlr:master Sep 10, 2020
@parrt parrt added this to the 4.9 milestone Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants