[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC322079: [ASTImporter] Fix missing SourceLoc import for 
ObjCMethodDecl selectors (authored by a.sidorin, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D6550

Files:
  lib/AST/ASTImporter.cpp
  test/ASTMerge/interface/Inputs/interface1.m


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
 ToParams[I]->setOwningFunction(ToMethod);
 ToMethod->addDeclInternal(ToParams[I]);
   }
+
   SmallVector SelLocs;
   D->getSelectorLocs(SelLocs);
-  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs); 
+  for (SourceLocation  : SelLocs)
+Loc = Importer.Import(Loc);
+
+  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
 
   ToMethod->setLexicalDeclContext(LexicalDC);
   Importer.Imported(D, ToMethod);
Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
 @implementation I15 : I12
 @end
 
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
 ToParams[I]->setOwningFunction(ToMethod);
 ToMethod->addDeclInternal(ToParams[I]);
   }
+
   SmallVector SelLocs;
   D->getSelectorLocs(SelLocs);
-  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs); 
+  for (SourceLocation  : SelLocs)
+Loc = Importer.Import(Loc);
+
+  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
 
   ToMethod->setLexicalDeclContext(LexicalDC);
   Importer.Imported(D, ToMethod);
Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
 @implementation I15 : I12
 @end
 
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Great! Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin updated this revision to Diff 127375.
a.sidorin added reviewers: xazax.hun, szepet.
a.sidorin added a comment.
Herald added a subscriber: rnkovacs.

Removed already fixed stuff, added a test for remaining.


Repository:
  rC Clang

https://reviews.llvm.org/D6550

Files:
  lib/AST/ASTImporter.cpp
  test/ASTMerge/interface/Inputs/interface1.m


Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
 @implementation I15 : I12
 @end
 
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
 ToParams[I]->setOwningFunction(ToMethod);
 ToMethod->addDeclInternal(ToParams[I]);
   }
+
   SmallVector SelLocs;
   D->getSelectorLocs(SelLocs);
-  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs); 
+  for (SourceLocation  : SelLocs)
+Loc = Importer.Import(Loc);
+
+  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
 
   ToMethod->setLexicalDeclContext(LexicalDC);
   Importer.Imported(D, ToMethod);


Index: test/ASTMerge/interface/Inputs/interface1.m
===
--- test/ASTMerge/interface/Inputs/interface1.m
+++ test/ASTMerge/interface/Inputs/interface1.m
@@ -100,4 +100,6 @@
 @implementation I15 : I12
 @end
 
-
+@interface ImportSelectorSLoc { }
+-(int)addInt:(int)a toInt:(int)b moduloInt:(int)c; // don't crash here
+@end
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
 ToParams[I]->setOwningFunction(ToMethod);
 ToMethod->addDeclInternal(ToParams[I]);
   }
+
   SmallVector SelLocs;
   D->getSelectorLocs(SelLocs);
-  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs); 
+  for (SourceLocation  : SelLocs)
+Loc = Importer.Import(Loc);
+
+  ToMethod->setMethodParams(Importer.getToContext(), ToParams, SelLocs);
 
   ToMethod->setLexicalDeclContext(LexicalDC);
   Importer.Imported(D, ToMethod);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Thank you Sean, I'll try.


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Sean Callanan via Phabricator via cfe-commits]

spyffe added a comment.

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.


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

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.


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

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.


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D6550#663002, @a.sidorin wrote:

> 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?


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D6550: ASTImporter: Fix missing SourceLoc imports

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

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




Comment at: lib/AST/ASTImporter.cpp:2749
   // Create the imported function.
+  SourceLocation StartLoc = Importer.Import(D->getInnerLocStart());
   TypeSourceInfo *TInfo = Importer.Import(D->getTypeSourceInfo());

This chunk should be fixed by r236012.



Comment at: lib/AST/ASTImporter.cpp:3310
   D->getSelectorLocs(SelLocs);
+  for (SourceLocation  : SelLocs)
+Loc = Importer.Import(Loc);

Still actual.



Comment at: lib/AST/ASTImporter.cpp:4613
+  return ToContext.getTrivialTypeSourceInfo(
+  T, Import(FromTSI->getTypeLoc().getLocStart()));
 }

Fixed by r241542.


https://reviews.llvm.org/D6550



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits