[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

The fix looks OK (other alternative is to remove the CHECK with CXXRecordDecl, 
or make a single line with regular expressions).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

This change caused the Import/namespace/struct-and-var/test.cpp test to fail on 
ARM due to an extra line with `-CXXRecordDecl being emitted by the compiler 
that was being matched instead of the intended line. I checked in a fix to 
tighten up the check a little more so that it gets the correct line in 757bc55 
. I don't 
think it should negatively affect the test, but please do review the change.

Sample test failure from build bot:
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1131/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Atest.cpp

   TEST 'Clang :: Import/struct-and-var/test.cpp' FAILED 

  Script:
  --
  : 'RUN: at line 1';   clang-import-test -dump-ast --import 
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp
 --import 
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp
 -expression 
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp
 | c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe 
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "RUN: at line 1"
  $ "clang-import-test" "-dump-ast" "--import" 
"C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp"
 "--import" 
"C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp"
 "-expression" 
"C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
  $ "c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe" 
"C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
  # command stderr:
  
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp:4:16:
 error: CHECK-SAME: is not on the same line as the previous match
  
  // CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F
  
 ^
  
  :49:126: note: 'next' match was here
  
  `-CXXRecordDecl 0x63d9992750 
 line:1:8 struct F definition
  

   ^
  
  :25:18: note: previous match ended here
  
  | `-CXXRecordDecl 0x63d9992378 <>  implicit 
 struct __va_list definition
  
   ^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa9f10ebffaa2: [ASTImporter] Various source location and 
range import fixes. (authored by balazske).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/test/Import/cxx-anon-namespace/test.cpp
  clang/test/Import/enum/test.cpp
  clang/test/Import/namespace/Inputs/NS.cpp
  clang/test/Import/namespace/test.cpp
  clang/test/Import/struct-and-var/test.cpp
  clang/test/Import/template-specialization/test.cpp

Index: clang/test/Import/template-specialization/test.cpp
===
--- clang/test/Import/template-specialization/test.cpp
+++ clang/test/Import/template-specialization/test.cpp
@@ -1,4 +1,7 @@
-// RUN: clang-import-test -import %S/Inputs/T.cpp -expression %s
+// RUN: clang-import-test -dump-ast -import %S/Inputs/T.cpp -expression %s | FileCheck %s
+
+// CHECK: |-ClassTemplateSpecializationDecl
+// CHECK-SAME:  line:4:20 struct A
 
 void expr() {
   A::B b1;
Index: clang/test/Import/struct-and-var/test.cpp
===
--- clang/test/Import/struct-and-var/test.cpp
+++ clang/test/Import/struct-and-var/test.cpp
@@ -1,4 +1,8 @@
-// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+// RUN: clang-import-test -dump-ast --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s | FileCheck %s
+
+// CHECK: `-CXXRecordDecl
+// CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F
+
 void expr() {
   struct F f;
   int x = f.a;
Index: clang/test/Import/namespace/test.cpp
===
--- /dev/null
+++ clang/test/Import/namespace/test.cpp
@@ -0,0 +1,8 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/NS.cpp -expression %s | FileCheck %s
+
+// CHECK: `-NamespaceDecl
+// CHECK-SAME: Inputs/NS.cpp:1:1, line:5:1> line:1:11 NS
+
+void expr() {
+  static_assert(NS::A == 3);
+}
Index: clang/test/Import/namespace/Inputs/NS.cpp
===
--- /dev/null
+++ clang/test/Import/namespace/Inputs/NS.cpp
@@ -0,0 +1,5 @@
+namespace NS {
+void f1();
+void f2();
+const int A = 3;
+}; // namespace NS
Index: clang/test/Import/enum/test.cpp
===
--- clang/test/Import/enum/test.cpp
+++ clang/test/Import/enum/test.cpp
@@ -1,5 +1,7 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/S.cpp -expression %s | FileCheck %s
 
+// CHECK: |-EnumDecl
+// CHECK-SAME: Inputs/S.cpp:1:1, line:4:1> line:1:6 E
 // CHECK: OpaqueWithType 'long'
 
 void expr() {
Index: clang/test/Import/cxx-anon-namespace/test.cpp
===
--- clang/test/Import/cxx-anon-namespace/test.cpp
+++ clang/test/Import/cxx-anon-namespace/test.cpp
@@ -8,7 +8,7 @@
 // CHECK: F.cpp:1:1
 // The nested anonymous namespace.
 // CHECK-NEXT: NamespaceDecl
-// CHECK-SAME: 
+// CHECK-SAME: line:21:11
 // CHECK: FunctionDecl
 // CHECK-SAME: func4
 // CHECK-NEXT: CompoundStmt
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2228,6 +2228,9 @@
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
@@ -2237,6 +2240,7 @@
 *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
 /*PrevDecl=*/nullptr))
   return ToNamespace;
+ToNamespace->setRBraceLoc(*RBraceLocOrErr);
 ToNamespace->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(ToNamespace);
 
@@ -2545,9 +2549,10 @@
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
-  if (auto Imp = importSeq(
-  D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
-std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType) = *Imp;
+  SourceRange ToBraceRange;
+  if (auto Imp = importSeq(D->getBeginLoc(), D->getQualifierLoc(),
+   D->getIntegerType(), D->getBraceRange()))
+std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType, ToBraceRange) = *Imp;
   else
 return Imp.takeError();
 
@@ -2561,6 +2566,7 @@
 
   D2->setQualifierInfo(ToQualifierLoc);
   D2->setIntegerType(ToIntegerType);
+  D2->setBraceRange(ToBraceRange);
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(D2);
@@ -2795,6 +2801,10 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 232097.
balazske added a comment.

Rebased to monorepo and newer master.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/test/Import/cxx-anon-namespace/test.cpp
  clang/test/Import/enum/test.cpp
  clang/test/Import/namespace/Inputs/NS.cpp
  clang/test/Import/namespace/test.cpp
  clang/test/Import/struct-and-var/test.cpp
  clang/test/Import/template-specialization/test.cpp

Index: clang/test/Import/template-specialization/test.cpp
===
--- clang/test/Import/template-specialization/test.cpp
+++ clang/test/Import/template-specialization/test.cpp
@@ -1,4 +1,7 @@
-// RUN: clang-import-test -import %S/Inputs/T.cpp -expression %s
+// RUN: clang-import-test -dump-ast -import %S/Inputs/T.cpp -expression %s | FileCheck %s
+
+// CHECK: |-ClassTemplateSpecializationDecl
+// CHECK-SAME:  line:4:20 struct A
 
 void expr() {
   A::B b1;
Index: clang/test/Import/struct-and-var/test.cpp
===
--- clang/test/Import/struct-and-var/test.cpp
+++ clang/test/Import/struct-and-var/test.cpp
@@ -1,4 +1,8 @@
-// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+// RUN: clang-import-test -dump-ast --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s | FileCheck %s
+
+// CHECK: `-CXXRecordDecl
+// CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F
+
 void expr() {
   struct F f;
   int x = f.a;
Index: clang/test/Import/namespace/test.cpp
===
--- /dev/null
+++ clang/test/Import/namespace/test.cpp
@@ -0,0 +1,8 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/NS.cpp -expression %s | FileCheck %s
+
+// CHECK: `-NamespaceDecl
+// CHECK-SAME: Inputs/NS.cpp:1:1, line:5:1> line:1:11 NS
+
+void expr() {
+  static_assert(NS::A == 3);
+}
Index: clang/test/Import/namespace/Inputs/NS.cpp
===
--- /dev/null
+++ clang/test/Import/namespace/Inputs/NS.cpp
@@ -0,0 +1,5 @@
+namespace NS {
+void f1();
+void f2();
+const int A = 3;
+}; // namespace NS
Index: clang/test/Import/enum/test.cpp
===
--- clang/test/Import/enum/test.cpp
+++ clang/test/Import/enum/test.cpp
@@ -1,5 +1,7 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/S.cpp -expression %s | FileCheck %s
 
+// CHECK: |-EnumDecl
+// CHECK-SAME: Inputs/S.cpp:1:1, line:4:1> line:1:6 E
 // CHECK: OpaqueWithType 'long'
 
 void expr() {
Index: clang/test/Import/cxx-anon-namespace/test.cpp
===
--- clang/test/Import/cxx-anon-namespace/test.cpp
+++ clang/test/Import/cxx-anon-namespace/test.cpp
@@ -8,7 +8,7 @@
 // CHECK: F.cpp:1:1
 // The nested anonymous namespace.
 // CHECK-NEXT: NamespaceDecl
-// CHECK-SAME: 
+// CHECK-SAME: line:21:11
 // CHECK: FunctionDecl
 // CHECK-SAME: func4
 // CHECK-NEXT: CompoundStmt
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2228,6 +2228,9 @@
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
@@ -2237,6 +2240,7 @@
 *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
 /*PrevDecl=*/nullptr))
   return ToNamespace;
+ToNamespace->setRBraceLoc(*RBraceLocOrErr);
 ToNamespace->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(ToNamespace);
 
@@ -2545,9 +2549,10 @@
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
-  if (auto Imp = importSeq(
-  D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
-std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType) = *Imp;
+  SourceRange ToBraceRange;
+  if (auto Imp = importSeq(D->getBeginLoc(), D->getQualifierLoc(),
+   D->getIntegerType(), D->getBraceRange()))
+std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType, ToBraceRange) = *Imp;
   else
 return Imp.takeError();
 
@@ -2561,6 +2566,7 @@
 
   D2->setQualifierInfo(ToQualifierLoc);
   D2->setIntegerType(ToIntegerType);
+  D2->setBraceRange(ToBraceRange);
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(D2);
@@ -2795,6 +2801,10 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping @shafik @balazske


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Added lit tests for the simple (`Decl`) cases. The `Expr` and type related 
changes are more tricky to do. But I do not like this approach to add new tests 
because the test function in D60463  does 
almost the same thing for every import in ASTTests without adding lot of new 
checks manually.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 202438.
balazske added a comment.

- Import BraceRange of EnumDecl.
- Added lit tests.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499

Files:
  lib/AST/ASTImporter.cpp
  test/Import/enum/test.cpp
  test/Import/namespace/Inputs/NS.cpp
  test/Import/namespace/test.cpp
  test/Import/struct-and-var/test.cpp
  test/Import/template-specialization/test.cpp

Index: test/Import/template-specialization/test.cpp
===
--- test/Import/template-specialization/test.cpp
+++ test/Import/template-specialization/test.cpp
@@ -1,4 +1,7 @@
-// RUN: clang-import-test -import %S/Inputs/T.cpp -expression %s
+// RUN: clang-import-test -dump-ast -import %S/Inputs/T.cpp -expression %s | FileCheck %s
+
+// CHECK: |-ClassTemplateSpecializationDecl
+// CHECK-SAME:  line:4:20 struct A
 
 void expr() {
   A::B b1;
Index: test/Import/struct-and-var/test.cpp
===
--- test/Import/struct-and-var/test.cpp
+++ test/Import/struct-and-var/test.cpp
@@ -1,4 +1,8 @@
-// RUN: clang-import-test --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s
+// RUN: clang-import-test -dump-ast --import %S/Inputs/S1.cpp --import %S/Inputs/S2.cpp -expression %s | FileCheck %s
+
+// CHECK: `-CXXRecordDecl
+// CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F
+
 void expr() {
   struct F f;
   int x = f.a;
Index: test/Import/namespace/test.cpp
===
--- /dev/null
+++ test/Import/namespace/test.cpp
@@ -0,0 +1,8 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/NS.cpp -expression %s | FileCheck %s
+
+// CHECK: `-NamespaceDecl
+// CHECK-SAME: Inputs/NS.cpp:1:1, line:5:1> line:1:11 NS
+
+void expr() {
+  static_assert(NS::A == 3);
+}
Index: test/Import/namespace/Inputs/NS.cpp
===
--- /dev/null
+++ test/Import/namespace/Inputs/NS.cpp
@@ -0,0 +1,5 @@
+namespace NS {
+  void f1();
+  void f2();
+  const int A = 3;
+};
Index: test/Import/enum/test.cpp
===
--- test/Import/enum/test.cpp
+++ test/Import/enum/test.cpp
@@ -1,5 +1,7 @@
 // RUN: clang-import-test -dump-ast -import %S/Inputs/S.cpp -expression %s | FileCheck %s
 
+// CHECK: |-EnumDecl
+// CHECK-SAME: Inputs/S.cpp:1:1, line:4:1> line:1:6 E
 // CHECK: OpaqueWithType 'long'
 
 void expr() {
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2158,6 +2158,9 @@
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
@@ -2167,6 +2170,7 @@
 *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
 /*PrevDecl=*/nullptr))
   return ToNamespace;
+ToNamespace->setRBraceLoc(*RBraceLocOrErr);
 ToNamespace->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(ToNamespace);
 
@@ -2464,9 +2468,10 @@
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
-  if (auto Imp = importSeq(
-  D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
-std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType) = *Imp;
+  SourceRange ToBraceRange;
+  if (auto Imp = importSeq(D->getBeginLoc(), D->getQualifierLoc(),
+   D->getIntegerType(), D->getBraceRange()))
+std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType, ToBraceRange) = *Imp;
   else
 return Imp.takeError();
 
@@ -2480,6 +2485,7 @@
 
   D2->setQualifierInfo(ToQualifierLoc);
   D2->setIntegerType(ToIntegerType);
+  D2->setBraceRange(ToBraceRange);
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(D2);
@@ -2712,6 +2718,10 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
   if (auto QualifierLocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*QualifierLocOrErr);
   else
@@ -5181,6 +5191,11 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
+
   // Import the qualifier, if any.
   if (auto LocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*LocOrErr);
@@ -6174,7 +6189,8 @@
   TemplateArgumentListInfo *ToResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) 

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

Actually I was mistaken, we can see the difference for `EnumDecl` and 
`ClassTemplateSpecializationDecl` as well.

For `EnumDecl` before:

  EnumDecl 0x7fd0ae884800  col:6 referenced B

After:

  EnumDecl 0x7fa703882600  line:2:6 referenced B

For `ClassTemplateSpecializationDecl` before:

  ClassTemplateSpecializationDecl 0x7f8c338846a0  col:7 
class A definition

after:

  ClassTemplateSpecializationDecl 0x7fca150832a0  line:5:7 
class A definition

So once we have tests similar to D61140  I 
would feel good about this change. Especially since it is not clear when the 
other changes will be in and tests are important to guard against regressions.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

It looks like that the same test can be applied as in D60463 
 but check only the first line of the AST 
dump. The first line contains information about the actual Decl only. This 
checks less than checking the full AST dump but finds some of wrong 
`SourceLocation` import. The problem is that the test can not be added yet 
because there are other failures. At least one new patch is needed with 
corrections related to import of `TypeSourceInfo`. Anyway this change (and the 
others) can not be added together with the test because multiple patches 
(including this) are needed to make the test not fail.

Source locations can be observed here:
http://ce.steveire.com/z/WB9VAu


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I think the AST dump for `EnumDecl` and `ClassTemplateSpecializationDecl` 
should be dumping the missing `SourceLocations` but I am having trouble 
tracking down where that should be done.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

So an alternative to testing could be matching the AST output from a 
`clang-import-test` like we did here:

https://reviews.llvm.org/D61140

although it unfortunately looks like only only the AST dump of `NamespaceDecl` 
output the `SourceLocation` you are fixing, see it on godbolt 
 when I try a `clang-import-test` it looks like 
it is missing that `SourceLocation`:

  |-NamespaceDecl 0x7f8c1f884750 > col:11 

and with your patch applied I see the `SourceLocation`:

  |-NamespaceDecl 0x7fe3e6882f50  line:1:11 A


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6129
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))

shafik wrote:
> Curious why you decided to add the new arguments to the front as opposed to 
> the end?
This overload of `ImportTemplateArgumentListInfo` already exists before the 
patch. It looks like that the last argument is the output value and the 
arguments before are input values.



Comment at: lib/AST/ASTImporter.cpp:7150
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());

shafik wrote:
> Can you explain why `E->getNameInfo().getLoc()` is more correct than 
> `E->getExprLoc()`?
The getExprLoc returns (if I am correct) the begin location and 
`getNameInfo().getLoc()` returns in "X::value" location of "value" (this 
should not be the same as BeginLoc, may be it is the EndLoc?). (The begin and 
end location is not imported explicitly, it is obtained from other location 
values in the expression object.) I just observed that `E->getLocation()` can 
be used instead of `E->getNameInfo().getLoc()`.
(The reason why this is correct that the test in D60463 indicates failure if 
getExprLoc is used, the begin and end locations are the same. That test works 
only in our Ericsson fork because Decl reordering issue.)



Comment at: lib/AST/ASTImporter.cpp:7225
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;

shafik wrote:
> We still want to import a few lines down even if 
> `!E->getTemplateKeywordLoc().isValid()`?
The TemplateKeywordLoc can be invalid if the optional `template` keyword is 
missing. The import function can import an invalid SourceLocation (as invalid 
but not error).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

There is a test for the SourceLocation import in 
https://reviews.llvm.org/D60463 (after this patch is applied that test should 
not fail and the "return" statement is to be removed).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I don't see any regressions but I am a little uncomfortable since there are no 
tests. So I would feel better if this was split into three parts: Namespaces, 
Enums and Templates.

Are there small test programs that fail due to the missing data? We can add 
them as regression tests.




Comment at: lib/AST/ASTImporter.cpp:6129
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))

Curious why you decided to add the new arguments to the front as opposed to the 
end?



Comment at: lib/AST/ASTImporter.cpp:7150
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());

Can you explain why `E->getNameInfo().getLoc()` is more correct than 
`E->getExprLoc()`?



Comment at: lib/AST/ASTImporter.cpp:7225
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;

We still want to import a few lines down even if 
`!E->getTemplateKeywordLoc().isValid()`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499



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

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 201521.
balazske added a comment.

- Import BraceRange of EnumDecl.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60499/new/

https://reviews.llvm.org/D60499

Files:
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2146,6 +2146,9 @@
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
@@ -2155,6 +2158,7 @@
 *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
 /*PrevDecl=*/nullptr))
   return ToNamespace;
+ToNamespace->setRBraceLoc(*RBraceLocOrErr);
 ToNamespace->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(ToNamespace);
 
@@ -2452,9 +2456,10 @@
   SourceLocation ToBeginLoc;
   NestedNameSpecifierLoc ToQualifierLoc;
   QualType ToIntegerType;
-  if (auto Imp = importSeq(
-  D->getBeginLoc(), D->getQualifierLoc(), D->getIntegerType()))
-std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType) = *Imp;
+  SourceRange ToBraceRange;
+  if (auto Imp = importSeq(D->getBeginLoc(), D->getQualifierLoc(),
+   D->getIntegerType(), D->getBraceRange()))
+std::tie(ToBeginLoc, ToQualifierLoc, ToIntegerType, ToBraceRange) = *Imp;
   else
 return Imp.takeError();
 
@@ -2468,6 +2473,7 @@
 
   D2->setQualifierInfo(ToQualifierLoc);
   D2->setIntegerType(ToIntegerType);
+  D2->setBraceRange(ToBraceRange);
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(D2);
@@ -2697,6 +2703,10 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
   if (auto QualifierLocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*QualifierLocOrErr);
   else
@@ -5118,6 +5128,11 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
+
   // Import the qualifier, if any.
   if (auto LocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*LocOrErr);
@@ -6111,7 +6126,8 @@
   TemplateArgumentListInfo *ToResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))
   return std::move(Err);
 ToResInfo = 
   }
@@ -7130,20 +7146,18 @@
 
 ExpectedStmt
 ASTNodeImporter::VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E) {
-  auto Imp = importSeq(
-  E->getQualifierLoc(), E->getTemplateKeywordLoc(), E->getDeclName(),
-  E->getExprLoc(), E->getLAngleLoc(), E->getRAngleLoc());
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());
   if (!Imp)
 return Imp.takeError();
 
   NestedNameSpecifierLoc ToQualifierLoc;
-  SourceLocation ToTemplateKeywordLoc, ToExprLoc, ToLAngleLoc, ToRAngleLoc;
+  SourceLocation ToTemplateKeywordLoc, ToNameLoc, ToLAngleLoc, ToRAngleLoc;
   DeclarationName ToDeclName;
-  std::tie(
-  ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToExprLoc,
-  ToLAngleLoc, ToRAngleLoc) = *Imp;
-
-  DeclarationNameInfo ToNameInfo(ToDeclName, ToExprLoc);
+  std::tie(ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToNameLoc,
+   ToLAngleLoc, ToRAngleLoc) = *Imp;
+  DeclarationNameInfo ToNameInfo(ToDeclName, ToNameLoc);
   if (Error Err = ImportDeclarationNameLoc(E->getNameInfo(), ToNameInfo))
 return std::move(Err);
 
@@ -7208,7 +7222,7 @@
 else
   return ToDOrErr.takeError();
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;
 if (Error Err = ImportTemplateArgumentListInfo(
 E->getLAngleLoc(), E->getRAngleLoc(), E->template_arguments(),
@@ -7262,8 +7276,9 @@
   TemplateArgumentListInfo ToTAInfo;
   TemplateArgumentListInfo *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+TemplateArgumentListInfo FromTAInfo;
+E->copyTemplateArgumentsInto(FromTAInfo);
+if (Error Err = ImportTemplateArgumentListInfo(FromTAInfo, ToTAInfo))
   

[PATCH] D60499: [ASTImporter] Various source location and range import fixes.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.

ASTImporter contained wrong or missing imports of SourceLocation
and SourceRange for some objects. At least a part of such errors
is fixed now.
Tests for SourceLocation import do not exist yet. A separate
patch is needed to add a general SourceLocation import test
that runs on every import as part of the already existing tests.


Repository:
  rC Clang

https://reviews.llvm.org/D60499

Files:
  lib/AST/ASTImporter.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2146,6 +2146,9 @@
   ExpectedSLoc BeginLocOrErr = import(D->getBeginLoc());
   if (!BeginLocOrErr)
 return BeginLocOrErr.takeError();
+  ExpectedSLoc RBraceLocOrErr = import(D->getRBraceLoc());
+  if (!RBraceLocOrErr)
+return RBraceLocOrErr.takeError();
 
   // Create the "to" namespace, if needed.
   NamespaceDecl *ToNamespace = MergeWithNamespace;
@@ -2155,6 +2158,7 @@
 *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
 /*PrevDecl=*/nullptr))
   return ToNamespace;
+ToNamespace->setRBraceLoc(*RBraceLocOrErr);
 ToNamespace->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(ToNamespace);
 
@@ -2697,6 +2701,10 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
   if (auto QualifierLocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*QualifierLocOrErr);
   else
@@ -5118,6 +5126,11 @@
 LexicalDC->addDeclInternal(D2);
   }
 
+  if (auto BraceRangeOrErr = import(D->getBraceRange()))
+D2->setBraceRange(*BraceRangeOrErr);
+  else
+return BraceRangeOrErr.takeError();
+
   // Import the qualifier, if any.
   if (auto LocOrErr = import(D->getQualifierLoc()))
 D2->setQualifierInfo(*LocOrErr);
@@ -6111,7 +6124,8 @@
   TemplateArgumentListInfo *ToResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
 if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))
   return std::move(Err);
 ToResInfo = 
   }
@@ -7130,20 +7144,18 @@
 
 ExpectedStmt
 ASTNodeImporter::VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E) {
-  auto Imp = importSeq(
-  E->getQualifierLoc(), E->getTemplateKeywordLoc(), E->getDeclName(),
-  E->getExprLoc(), E->getLAngleLoc(), E->getRAngleLoc());
+  auto Imp = importSeq(E->getQualifierLoc(), E->getTemplateKeywordLoc(),
+   E->getDeclName(), E->getNameInfo().getLoc(),
+   E->getLAngleLoc(), E->getRAngleLoc());
   if (!Imp)
 return Imp.takeError();
 
   NestedNameSpecifierLoc ToQualifierLoc;
-  SourceLocation ToTemplateKeywordLoc, ToExprLoc, ToLAngleLoc, ToRAngleLoc;
+  SourceLocation ToTemplateKeywordLoc, ToNameLoc, ToLAngleLoc, ToRAngleLoc;
   DeclarationName ToDeclName;
-  std::tie(
-  ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToExprLoc,
-  ToLAngleLoc, ToRAngleLoc) = *Imp;
-
-  DeclarationNameInfo ToNameInfo(ToDeclName, ToExprLoc);
+  std::tie(ToQualifierLoc, ToTemplateKeywordLoc, ToDeclName, ToNameLoc,
+   ToLAngleLoc, ToRAngleLoc) = *Imp;
+  DeclarationNameInfo ToNameInfo(ToDeclName, ToNameLoc);
   if (Error Err = ImportDeclarationNameLoc(E->getNameInfo(), ToNameInfo))
 return std::move(Err);
 
@@ -7208,7 +7220,7 @@
 else
   return ToDOrErr.takeError();
 
-  if (E->hasExplicitTemplateArgs() && E->getTemplateKeywordLoc().isValid()) {
+  if (E->hasExplicitTemplateArgs()) {
 TemplateArgumentListInfo ToTAInfo;
 if (Error Err = ImportTemplateArgumentListInfo(
 E->getLAngleLoc(), E->getRAngleLoc(), E->template_arguments(),
@@ -7262,8 +7274,9 @@
   TemplateArgumentListInfo ToTAInfo;
   TemplateArgumentListInfo *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-if (Error Err =
-ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+TemplateArgumentListInfo FromTAInfo;
+E->copyTemplateArgumentsInto(FromTAInfo);
+if (Error Err = ImportTemplateArgumentListInfo(FromTAInfo, ToTAInfo))
   return std::move(Err);
 ResInfo = 
   }
@@ -8023,8 +8036,14 @@
 return std::move(Err);
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
 QualType(Spec->getAsType(), 0), ToTLoc);
-  Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
- ToLocalEndLoc);
+  if (Kind == NestedNameSpecifier::TypeSpecWithTemplate)
+// ToLocalBeginLoc is