This is an archive of the discontinued LLVM Phabricator instance.

ASTImporter: Fix missing SourceLoc imports
ClosedPublic

Authored by a.sidorin on Dec 5 2014, 6:16 AM.

Diff Detail

Repository
rC Clang

Event Timeline

nrieck updated this revision to Diff 16986.Dec 5 2014, 6:16 AM
nrieck retitled this revision from to ASTImporter: Fix missing SourceLoc imports.
nrieck updated this object.
nrieck edited the test plan for this revision. (Show Details)
nrieck added a subscriber: Unknown Object (MLST).
a.sidorin edited edge metadata.Feb 1 2017, 3:58 AM

Hi Gabor. One of the bugs fixed in this patch is still present in master, other two are already fixed.

lib/AST/ASTImporter.cpp
2153

This chunk should be fixed by r236012.

2863

Still actual.

6133

Fixed by r241542.

Hi Gabor. One of the bugs fixed in this patch is still present in master, other two are already fixed.

Thanks for checking that! Do you think it is ok for me to commit the missing part?

This patch lacks tests. If you add at least minimal test case (I'm not familiar with ObjC and its front-end, unfortunately), I will no have any concerns. Also adding Sean.

Will anybody object if I commit this change without a test? This bug seems to be pretty obvious but, unfortunately, I'm not familiar with Objective C.

spyffe edited edge metadata.Jun 16 2017, 11:11 AM

Hmm, the transforming in place of SelLocs reads a little weirdly to me, but other than that the code seems fine.
Is your concern that you don't know how to write an Objective-C test that would cover this? It looks to me like an Objective-C interface with a method:

@interface MyClass { }
-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c;
@end

might be enough for an ASTMerge test. If you want to make clang-import-test use this code, then we might need to add one or two things into that tester to handle Objective-C method lookup.

Thank you Sean, I'll try.

a.sidorin commandeered this revision.Dec 18 2017, 9:05 AM
a.sidorin edited reviewers, added: nrieck; removed: a.sidorin.
a.sidorin updated this revision to Diff 127375.Dec 18 2017, 9:10 AM
a.sidorin added reviewers: xazax.hun, szepet.

Removed already fixed stuff, added a test for remaining.

xazax.hun accepted this revision.Jan 9 2018, 3:05 AM

Great! Thanks!

This revision is now accepted and ready to land.Jan 9 2018, 3:05 AM
This revision was automatically updated to reflect the committed changes.