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

Exclude MSVC workaround on clang and gcc #1306

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Exclude MSVC workaround on clang and gcc #1306

merged 2 commits into from
Oct 10, 2024

Conversation

gregmarr
Copy link
Contributor

@gregmarr gregmarr commented Oct 9, 2024

Resolves #1304
On Windows, clang defines _MSC_VER, so the MSVC workaround needs to not happen when the clang define is set.

@gregmarr
Exclude MSVC workaround on clang and gcc
5d080a2 
Resolves hsutter#1304  
On Windows, clang defines `_MSC_VER`, so the MSVC workaround needs to not happen when the clang define is set.


Signed-off-by: gregmarr <gregmarr@users.noreply.github.com>
MatthieuHernandez
MatthieuHernandez reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@MatthieuHernandez MatthieuHernandez left a comment

Choose a reason for hiding this comment

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

What about the other #if defined(_MSC_VER) lines 1544 and 1935?

include/cpp2util.h Outdated Show resolved Hide resolved
@MatthieuHernandez MatthieuHernandez mentioned this pull request Oct 9, 2024
Closed
@hsutter @MatthieuHernandez
Update include/cpp2util.h
f1bc7e4 
Co-authored-by: Matthieu HERNANDEZ <25429726+MatthieuHernandez@users.noreply.github.com>
Signed-off-by: Herb Sutter <herb.sutter@gmail.com>
@hsutter
Copy link
Owner

hsutter commented Oct 10, 2024 •
edited
Loading

Cool, I was just making the identical change (for __clang_major__ only) and running regressions when I saw this. If regressions pass I'll discard mine and merge this. Thanks!

@hsutter
Copy link
Owner

hsutter commented Oct 10, 2024

I think the one thing this PR doesn't do though is re-run regression tests... however I just did so I'll merge this and then separately merge the regression test outputs (which changed only because of cpp2util.h line numbers in error messages).

Thanks!

@hsutter hsutter merged commit 51d0f56 into hsutter:main Oct 10, 2024
16 of 28 checks passed
@gregmarr
Copy link
Contributor Author

gregmarr commented Oct 10, 2024 •
edited
Loading

Weird that this would cause any line numbers to change unless there was a clang on Windows that is changing from the const line to the constexpr line.

Update: Looks like all those tests were already failing in main. The last time they passed was Aug 27th when #1250 was merged. https://github.com/hsutter/cppfront/actions/workflows/regression-tests.yml?page=2&query=branch%3Amain They started failing again with the next commit a minute later.

@gregmarr gregmarr deleted the patch-1 branch October 10, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatthieuHernandez MatthieuHernandez MatthieuHernandez left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to compile cpp2 enum
3 participants
@gregmarr @hsutter @MatthieuHernandez