Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C++] Fix const correctness in ATN and DFA #3530

Merged
merged 1 commit into from
Feb 12, 2022
Merged

[C++] Fix const correctness in ATN and DFA #3530

merged 1 commit into from
Feb 12, 2022

Conversation

jcking
Copy link
Collaborator

@jcking jcking commented Feb 7, 2022

This is a safe but large change that fixes const correctness in various places in the ATN and DFA. Historically things were passed around as std::shared_ptr<T> which allowed T to be modified even if it never was. This change fixes it so that most things are passed around as std::shared_ptr<const T>. This makes thread saftey analysis easier when simply reading.

I also replaced const std::shared_ptr<T>& with const T& in places where std::shared_ptr was not being copied and thus extending the lifetime of T.

@jcking
Copy link
Collaborator Author

jcking commented Feb 7, 2022

@mike-lischke

@jcking jcking force-pushed the cpp-shared_ptr-opt branch 2 times, most recently from 3a1ed5f to dc6b311 Compare February 7, 2022 20:42
@jcking jcking changed the base branch from master to dev February 8, 2022 04:01
mike-lischke
mike-lischke suggested changes Feb 8, 2022
View reviewed changes
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.

Great work! Just fix the few issues I mentioned and we are good to go.

Please refrain from moving code to header files, if there's not really an absolute compelling reason.

runtime/Cpp/runtime/src/atn/ATNConfig.h Outdated Show resolved Hide resolved
runtime/Cpp/runtime/src/atn/ATNDeserializer.cpp Outdated Show resolved Hide resolved
runtime/Cpp/runtime/src/atn/ATNDeserializer.cpp Outdated Show resolved Hide resolved
runtime/Cpp/runtime/src/atn/ATNDeserializer.cpp Outdated Show resolved Hide resolved
@jcking
Copy link
Collaborator Author

jcking commented Feb 8, 2022

Something is weird with the MacOS runtime. I need to get a signal handler installed that dumps the stacktrace when it fails.

@jcking jcking force-pushed the cpp-shared_ptr-opt branch 5 times, most recently from e3fffbd to 140094c Compare February 8, 2022 19:25
@jcking
Copy link
Collaborator Author

jcking commented Feb 8, 2022

Something is super weird. I am going to need to grab my Wife's personal mac to investigate. I'll bump this when I figure out whats up. Looks like SEGFAULT via nullptr, but its hard to tell.

@jcking jcking force-pushed the cpp-shared_ptr-opt branch 3 times, most recently from 592c801 to 53df497 Compare February 8, 2022 21:22
@jcking
Copy link
Collaborator Author

jcking commented Feb 9, 2022

I tickled some weird MacOS bug in their toolchain. I'm going to split this up into separate CLs to narrow down the root cause. Leaving this open for the timebeing for reference.

@jcking jcking force-pushed the cpp-shared_ptr-opt branch 3 times, most recently from 0bad220 to 2bc31fc Compare February 9, 2022 15:14
@jcking
Copy link
Collaborator Author

jcking commented Feb 9, 2022

Okay, I think its good now @mike-lischke , I didn't need to split. There is something super sketchy going on with Hasher and Comparer used by the standard library unordered containers on MacOS for ATNConfig. It appears MacOS current version resolves equality and hash function specializations differently from Linux/Windows standard library implementations.

I removed the changes to equality and hashing for ATNConfig to avoid tickling the bug until I can find a workaround. So the existing behavior is preserved.

@jcking
Copy link
Collaborator Author

jcking commented Feb 9, 2022

I accidentally squashed the change I was talking about. Updated, and tests are all green.

mike-lischke
mike-lischke approved these changes Feb 11, 2022
View reviewed changes
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.

This looks ok now. @parrt This patch can be merged.

runtime/Cpp/runtime/src/support/Arrays.h
Comment on lines +35 to +38
if (!a[i] && !b[i])
continue;
if (!a[i] || !b[i])
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.

Not an issue per se, just mentioning that these two checks can be combined into one:

     if (!a[i] != !b[i])
        continue;

@parrt parrt added this to the 4.10 milestone Feb 12, 2022
@parrt
Copy link
Member

parrt commented Feb 12, 2022

Thanks guys!

@parrt parrt merged commit 8f30d47 into antlr:dev Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KvanTTT KvanTTT KvanTTT left review comments

@mike-lischke mike-lischke mike-lischke approved these changes

Assignees
No one assigned
Labels
target:cpp
Projects
None yet
Milestone
4.10
Development

Successfully merging this pull request may close these issues.
4 participants
@jcking @parrt @KvanTTT @mike-lischke