This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatcher] Fix a ASTMatcher test failure on Windows.
ClosedPublic

Authored by hokein on May 18 2016, 7:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 57623.May 18 2016, 7:49 AM
hokein retitled this revision from to [ASTMatcher] Fix a ASTMatcher test failure on Windows..
hokein updated this object.
hokein added reviewers: alexfh, aaron.ballman.
hokein added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.May 18 2016, 7:57 AM

Hrm, I kind of worry about this masking bugs when delayed template parsing is enabled (which it is by default on MSVC-built versions of clang).

thakis added a subscriber: thakis.May 18 2016, 8:04 AM

I agree with Aaron. Maybe you could change Matcher.UnresolvedLookupExpr to call bar() from a new function foo() so that it gets instantiated?

hokein updated this revision to Diff 57631.May 18 2016, 8:15 AM
hokein edited edge metadata.

Only use -fno-delayed-template-parsing in the testcase.

aaron.ballman accepted this revision.May 18 2016, 8:16 AM
aaron.ballman edited edge metadata.

I like this approach much better, thank you. LGTM!

This revision is now accepted and ready to land.May 18 2016, 8:16 AM

Maybe you could change Matcher.UnresolvedLookupExpr to call bar() from a new function foo() so that it gets instantiated?

This will break the intention of the testcase. We don't want the function get instantiated here. Update the code now only restrict -fno-delayed-template-parsing in this test.

thakis added inline comments.May 18 2016, 8:19 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
187 ↗(On Diff #57631)

Doesn't referencing bar() after it's defined help too? If not, this is cool too -- maybe add a /*ExpectMatch=*/ in front of true so it's clear what the true means.

Ah, you answered my question while I was writing it.

Doesn't that mean whatever feature this test is testing is broken on Windows?

(If so, maybe add a FIXME comment to make things work without delayed template parsing. In any case, getting the bot green is the most important thing, so landing this as is is definitely fine.)

This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.May 18 2016, 8:52 AM
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
187 ↗(On Diff #57631)

Yes, please add an argument comment and a FIXME to the code that is known to be broken with delayed template parsing. Thanks!

hokein marked 2 inline comments as done.May 18 2016, 9:56 AM
hokein added inline comments.
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
187 ↗(On Diff #57631)