[PATCH] D121176: [ASTStructuralEquivalence] Add support for comparing ObjCCategoryDecl.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Volodymyr,
Unfortunately, I'm not familiar with Objective-C and its frontend but I have a 
question.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1989
+   ++ParamT1, ++ParamT2) {
+if (!IsStructurallyEquivalent(Context, *ParamT1, *ParamT2))
+  return false;

Should we check if the parameter count is different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121176

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

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Hello Shafik!
The patch looks good, I only have a few stylish nits.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5964
+  new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &context = FromTU->getASTContext();
+  context.setExternalSource(std::move(source));

Context (starting with capital).



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5980
+  // Import the definition of the created class.
+  llvm::Error err = findFromTU(Record)->Importer->ImportDefinition(Record);
+  EXPECT_FALSE((bool)err);

Err (starting with capital).


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

https://reviews.llvm.org/D78000



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

[PATCH] D75740: [ASTImporter] Corrected import of repeated friend declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balazs,
This look almost good to me except some comments inline.




Comment at: clang/lib/AST/ASTImporter.cpp:3635
 
+static std::tuple
+getFriendCountAndPosition(FriendDecl *FD) {

It's better to turn the tuple into a named struct (`FriendCountAndPosition`?) 
to make the code more readable.



Comment at: clang/lib/AST/ASTImporter.cpp:3636
+static std::tuple
+getFriendCountAndPosition(FriendDecl *FD) {
+  unsigned int FriendCount = 0;

`const FriendDecl *FD`?



Comment at: clang/lib/AST/ASTImporter.cpp:3639
+  llvm::Optional FriendPosition;
+  auto *RD = cast(FD->getLexicalDeclContext());
+  if (FD->getFriendType()) {

const?



Comment at: clang/lib/AST/ASTImporter.cpp:3640
+  auto *RD = cast(FD->getLexicalDeclContext());
+  if (FD->getFriendType()) {
+QualType TypeOfFriend = FD->getFriendType()->getType().getCanonicalType();

```if (const TypeSourceInfo *FriendType = FD->getFriendType()) {
QualType TypeOfFriend = FriendType->getType().getCanonicalType();
...```
?



Comment at: clang/lib/AST/ASTImporter.cpp:3699
+
+  assert(ImportedEquivalentFriends.size() <= std::get<0>(CountAndPosition) &&
+ "Class with non-matching friends is imported, ODR check wrong?");

Why not strictly equal?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4009
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendType) {
+  auto Code =
+  R"(

clang-tidy wants this to be `const auto* Code`.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4034
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendDecl) {
+  auto Code =
+  R"(

clang-tidy wants this to be `const auto* Code`.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:801
+TEST_F(StructuralEquivalenceRecordTest, SameFriendMultipleTimes) {
+  auto t = makeNamedDecls("struct foo{ friend class X; };",
+  "struct foo{ friend class X; friend class X; };",

We need to place spaces after `foo`.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:811
+  Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(t));
+}

Could you please add a positive test with two `struct foo{ friend class X; 
friend class Y; };",` structures as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75740



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

[PATCH] D75732: [ASTImporter] Added visibility check for variable templates.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin 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/D75732/new/

https://reviews.llvm.org/D75732



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

[PATCH] D74554: [ASTImporter] Added visibility check for scoped enums.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4870
+  Decl *ToTU = getToTuDecl(
+  R"(
+  enum class E;

Maybe it's better to use just non-raw string literals here?
```
Decl *ToTU = getToTuDecl("enum class E;");
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74554



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

[PATCH] D74542: [ASTImporter] Prevent the ASTImporter from creating multiple main FileIDs.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Nice solution!


Repository:
  rC Clang

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

https://reviews.llvm.org/D74542



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

[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

[Aleksei Sidorin via Phabricator via cfe-commits]

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

The patch looks good. Thanks! I would prefer to see more tests for other 
TypeLoc cases, but I think they could be added in the following patches.
@shafik Shafik, do you have any suggestions for this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71018



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

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71020



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

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
I have an inline comment for the patch. Otherwise, looks good.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:252
+  QualType Ty = FD->getFriendType()->getType().getCanonicalType();
+  if (isa(Ty))
+Ty = cast(Ty)->getNamedType();

I wonder if ElaboratedType can be a canonical type because it is only a sugar 
type itself. Do we need to handle this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71020



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

[PATCH] D71018: [ASTImporter] Improved import of TypeSourceInfo (TypeLoc)

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Finally, this FIXME is addressed. Thanks! Are you going to add any tests in the 
following patches?
It looks almost good to me, but I have a question inline.




Comment at: clang/lib/AST/ASTImporter.cpp:7914
+
+#define IMPORT_TYPE_LOC(NAME)  
\
+  if (auto ImpOrErr = Importer.Import(From.get##NAME()))   
\

IMPORT_TYPE_LOC_ELEMENT_OR_RETURN_ERROR.



Comment at: clang/lib/AST/ASTImporter.cpp:8043
+
+  Error VisitFunctionTypeLoc(FunctionTypeLoc From) {
+auto To = ToL.castAs();

Does this import interacts well with type loc import partially done at L3258 
(VisitFunctionDecl)? Should we merge them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71018



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

[PATCH] D71020: [ASTImporter] Friend class decl should not be visible in its context

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
This patch looks mostly good to me. Thanks!




Comment at: clang/lib/AST/ASTImporter.cpp:334
+  FromDC->lookup(FromNamed->getDeclName());
+  if (std::find(FromLookup.begin(), FromLookup.end(), FromNamed) !=
+  FromLookup.end())

llvm::is_contained?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:258
+static const RecordDecl *getRecordDeclOfFriend(FriendDecl *FD) {
+  QualType Ty = FD->getFriendType()->getType();
+  if (auto *Inner = dyn_cast(Ty.getTypePtr())) {

Why don't we use getCanonicalType() anymore?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71020



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

[PATCH] D70819: [ASTImporter] Support functions with placeholder return types ...

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks!
Let's wait for Shafik's comments before committing this.




Comment at: clang/lib/AST/ASTImporter.cpp:48
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/ExceptionSpecificationType.h"

Do we use this new include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70819



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

[PATCH] D70819: [ASTImporter] Support functions with placeholder return types ...

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
That's an interesting case, thank for fixing this!




Comment at: clang/lib/AST/ASTImporter.cpp:3008
+// which is equal to the given DC.
+bool isAncestorDeclContextOf(DeclContext *DC, Decl *D) {
+  DeclContext *DCi = D->getDeclContext();

 ASTImporter is not very const-friendly, but this function is pure, so I think 
it's better to const-qualify the parameters.



Comment at: clang/lib/AST/ASTImporter.cpp:3020
+  QualType FromTy = D->getType();
+  const FunctionProtoType *FromFPT = FromTy->getAs();
+  if (AutoType *AutoT = FromFPT->getReturnType()->getContainedAutoType()) {

Is it possible for getAs() to return nullptr at this point?



Comment at: clang/lib/AST/ASTImporter.cpp:3174
+  bool UsedDifferentProtoType = false;
+  const auto *FromFPT = FromTy->getAs();
+  if (FromFPT) {

If FromFPT is not used outside of the condition, we can move the initialization 
inside if().



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5609
+  R"(
+  auto X = [](long l){
+using int_type = long;

Nit: a space before '{'.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5625
+  // parsed libcxx/src/filesystem/directory_iterator.cpp, but could not reduce
+  // that with creduce, because after preprocessing, the AST no longer
+  // contained the TypeAlias as a return type of the lambda.

That's interesting. Have you tried '-frewrite-includes' for expanding 
inclusions only without macro expansion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70819



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

[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Ouch! Sorry, Gabor -_\\


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68634



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

[PATCH] D68634: [ASTImporter] Imported FunctionDecls inherit attributes from existing functions

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the 
stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It 
will allow us not to miss the required actions while importing a new function 
too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68634



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

[PATCH] D67490: [Clang][ASTImporter] Added visibility check for FunctionTemplateDecl.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67490



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

[PATCH] D66336: [ASTImporter] Add development internals docs

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66336



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

[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Looks good, thank you for addressing the comments!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66538



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

[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66866



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

[PATCH] D66565: [analyzer] pr43036: Fix support for operator `sizeof...'.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

LGTM++


Repository:
  rC Clang

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

https://reviews.llvm.org/D66565



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

[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balasz,
I have some comments inline.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1579
+  D2 = D2->getCanonicalDecl();
+  std::pair P = std::make_pair(D1, D2);
+

`std::pair P{D1, D2}`?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1888
 
-Decl *D2 = TentativeEquivalences[D1];
-assert(D2 && "Unrecorded tentative equivalence?");
+Decl *D1 = P.first;
+Decl *D2 = P.second;

I would prefer std::tie instead: `std::tie(D1, D2) = P;`. But it is a matter of 
taste.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1290
+  bool isInNonEqCache(std::tuple D) {
+return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+   NonEquivalentDecls.end();

Can we use std::pair instead of std::tuple in this class? The size of tuple is 
known to be 2 and it will help to avoid such conversions.



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1291
+return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+   NonEquivalentDecls.end();
+  }

`return NonEquivalentDecls.count({get<0>(D), get<1>(D)});`?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1314
+  bool Eq = Ctx.IsEquivalent(get<0>(X), get<1>(X));
+  EXPECT_FALSE(Eq);
+

It seems to me that the assertion will become a bit easier to read without an 
intermediate variable. What do you think?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1357
+
+  EXPECT_FALSE(isInNonEqCache(C));
+  EXPECT_FALSE(isInNonEqCache(findDeclPair(

The declarations of C are not equivalent, but they are not placed into the 
non-equivalence cache. This looks strange to me, could you please explain this 
behavior?



Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1399
+  TU, cxxRecordDecl(hasName("A"), unless(isImplicit());
+  EXPECT_FALSE(isInNonEqCache(
+  findDeclPair(TU, functionDecl(hasName("x");

We don't have any tests where isInNonEqCache() can return true. Is it expected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66538



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

[PATCH] D66336: [ASTImporter] Add development internals docs

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: clang/docs/InternalsManual.rst:1470
+*templated* class (the ``CXXRecordDecl``) of a ``ClassTemplateDecl`` with
+``ClassTemplateDecl::getTemplatedDec()``. And we can get back a pointer of the
+"described" class template from the *templated* class:

martong wrote:
> a_sidorin wrote:
> > TemplatedDec_l_?
> Could you please elaborate, I don't get the meaning of the comment.
Sorry, I mean, you probably meant `ClassTemplateDecl::getTemplatedDecl` but 
typed `getTemplatedDec()` (missing 'l' at the end). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66336



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

[PATCH] D65182: [analyzer] Add fix-it hint support.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Artem!
The patch looks very promising for CSA, thanks for working on this!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:343
+  /// to produce a good fix-it hint for most path-sensitive warnings.
+  void addFixItHint(FixItHint F) {
+Fixits.push_back(F);

As I see, FixItHint is a pretty expensive class to copy. Should we consider 
const ref/move?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3100
+R->addRange(SR);
+  for (auto FH : Fixits)
+R->addFixItHint(FH);

const auto&/auto&&?



Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:741
+  o << "  \n";
+  for (auto Hint : D->getFixits()) {
+assert(!Hint.isNull());

const auto&?


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

https://reviews.llvm.org/D65182



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

[PATCH] D66336: [ASTImporter] Add development internals docs

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
This doc will become extremely useful for new developers. Thank you for dumping 
this sacred knowledge!




Comment at: clang/docs/InternalsManual.rst:1470
+*templated* class (the ``CXXRecordDecl``) of a ``ClassTemplateDecl`` with
+``ClassTemplateDecl::getTemplatedDec()``. And we can get back a pointer of the
+"described" class template from the *templated* class:

TemplatedDec_l_?



Comment at: clang/docs/InternalsManual.rst:1528
+
+When we compare two classes or enums and one of them is incomplete or has
+unloaded external lexical declarations then we cannot descend to compare their

I think we can extend this to point that import of all named declarations is 
preceded by name lookup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66336



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

[PATCH] D66348: [ASTImporter] Do not look up lambda classes

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor!
I think it's a correct solution for the analyzer: usually, we cannot import a 
lambda until we have to import some enclosing expression - which means that the 
lambdas are actually not the same. But I'm not so sure about how it can affect 
the LLDB logic. @shafik Shafik, could you please take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66348



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

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3293
 
+  // Import Ctor initializers.
+  if (auto *FromConstructor = dyn_cast(D)) {

I suggest to move it closer to the function body import because import of ctor 
initializers is a part of function body import in fact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65935



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

[PATCH] D65573: Add User docs for ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

That's incredible. Thank you!




Comment at: cfe/trunk/docs/LibASTImporter.rst:215
+Node *Result =
+const_cast(MatchRes[0].template getNodeAs("bindStr"));
+assert(Result);

We can avoid const_cast if we change the example function signature to `const 
Node *`.



Comment at: cfe/trunk/docs/LibASTImporter.rst:258
+
+We may extend the ``CmakeLists.txt`` under let's say ``clang/tools`` with the 
build and link instructions:
+

CMakeLists.txt



Comment at: cfe/trunk/docs/LibASTImporter.rst:557
+
+Now, Let's create an object file from the merged AST:
+

let's (small letter).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65573



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

[PATCH] D65999: [ASTImporter] Import additional flags for functions.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Balazs,
The change looks good. I think it can be committed after an unrelated part is 
removed.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5373
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
+::testing::Values(ArgVector{"-target",

This is a separate functional change and I prefer to move it into a separate 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65999



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

[PATCH] D65269: [ASTImporter] Fix for import of friend class template with definition.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D65269



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

[PATCH] D65999: [ASTImporter] Import additional flags for functions.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balazs,
The patch looks good in general.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5239
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportOfDefaultImplicitFunctions) {
+  // Test that import of implicit functions works and the functions

balazske wrote:
> martong wrote:
> > I don't exactly see how this test is related.
> I do not remember exactly why this test was added but probably the problem is 
> structural equivalence related: The flags are not imported correctly for the 
> first time, and at the second import structural match fails and a new Decl is 
> created instead of returning the existing one. This test fails if the change 
> is not applied.
Should we consider isExplicitlyDefaulted() when computing structural 
equivalence?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5242
+  // are merged into one chain.
+  auto GetDeclToImport = [this](const char *File) {
+Decl *FromTU = getTuDecl(

`const char *` -> `StringRef`?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5246
+struct X { };
+void f() { X x1, x2; x1 = x2; X *x3 = new X; delete x3; }
+)",

Can we remove the function body or reduce it to 'X x'?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65999



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

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balazs,
Do I understand correctly that it was unset 
`ToFunction->setLexicalDeclContext(LexicalDC);` that caused lookup problems?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65935



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

[PATCH] D65203: [ASTImporter] Do not import FunctionTemplateDecl in record twice.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65203



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

[PATCH] D65180: [analyzer] VirtualCallChecker: Improve warning messages.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Hi Artem,
The patch looks good to me. I prefer a fully qualified name, however, but it is 
a matter of taste.


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

https://reviews.llvm.org/D65180



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Gabor,
Thank you again for working on this patch. I think it can be committed after 
minor stylish issues are fixed.




Comment at: clang/lib/AST/ASTImporter.cpp:1677
+  // automatically load all the fields by calling
+  // LoadFieldsFromExternalStorage().  LoadFieldsFromExternalStorage() would
+  // call ASTImporter::Import(). This is because the ExternalASTSource

Two spaces after dot.



Comment at: clang/lib/AST/ASTImporter.cpp:1685
+return ChildErrors;
+  auto ToDCOrErr = Importer.ImportContext(FromDC);
+  if (!ToDCOrErr) {

Could you please add a newline after `return`?



Comment at: clang/lib/AST/ASTImporter.cpp:1696
+if (isa(D) || isa(D)) {
+  assert(D && "DC has a contained decl which is null?");
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);

"DC contains a null decl"?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5244
+}
+
+

A redundant newline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44100



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

[PATCH] D64241: [ASTImporter] Fix inequivalence of ClassTemplateInstantiations

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Looks good! Sorry for the delay :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64241



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

[PATCH] D64241: [ASTImporter] Fix inequivalence of ClassTemplateInstantiations

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
This looks fine, but could you please add a test showing how decl shadowing is 
handled? I.e. if we have Arg in one TU and both Arg and N::Arg in another TU.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64241



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hello Gabor,
This patch looks good to me.
Regarding the related patch: can we use getLambdaManglingNumber() for the 
comparison?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D60461: [ASTImporter] Import TemplateParameterLists in function templates.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Post-LGTM :)




Comment at: lib/AST/ASTImporter.cpp:2789
+  if (Num == 0)
+return Error::success();
+  SmallVector ToTPLists(Num);

Please add a newline after return.



Comment at: lib/AST/ASTImporter.cpp:2796
+else
+  return ToTPListOrErr.takeError();
+  ToD->setTemplateParameterListsInfo(Importer.ToContext, ToTPLists);

Let'a add a newline here too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60461



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

[PATCH] D62484: [ASTImporter] Added visibility context check for EnumDecl.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D62484



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
The patch looks good, but it looks to me that it has a relation to 
https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's delay 
landing this patch until the fix direction is clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64078: [ASTImporter] Fix structural ineq of lambdas with different sloc

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
it is a nice design question if source locations can participate in structural 
match or not. The comparison tells us that the same code written in different 
files is not structurally equivalent and I cannot agree with it. They can be 
not the same, but their structure is the same. The question is: why do we get 
to this comparison? Do we find a non-equivalent decl by lookup? I guess there 
can be another way to resolve this issue, and I'll be happy to help if you 
share what is the problem behind the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64078



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

[PATCH] D64073: [ASTImporter] Fix import of lambda in function param

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
There is an inline question about tests; other code looks fine.




Comment at: clang/lib/AST/ASTImporter.cpp:1713
+// In case of lambdas, the class already has a definition ptr set, but
+// the  contained decls are not imported yet. Also, isBeingDefined was
+// set in CXXRecordDecl::CreateLambda.  We must import the contained

Two spaces after 'the'.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5103
+  // count.
+  for (auto &D : ToL->decls()) {
+(void)D;

Can we use std::distance instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64073



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
This looks mostly good but I have a question inline.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1182
 
+  if (D1CXX->isLambda()) {
+if (!D2CXX->isLambda())

Should we return false if `D1CXX->isLambda() != D2CXX->isLambda()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp:173
+DE2->getQualifier());
+  } else if (auto CastE1 = dyn_cast(E1)) {
+auto *CastE2 = dyn_cast(E2);

Hi Gabor,
Is there any test for this branch?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62329



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

[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

> The following happened: During the analysis we compared two Decls which 
> turned out to be inequivalent, so we cached them. Later during the analysis, 
> however, we added a new node to the redecl chain of one of these Decls which 
> we previously compared. Then another structural equivalent check followed for 
> the two Decls. And this time they should have been considered structurally 
> equivalent, but the cache already contained them as nonequivalent. This 
> resulted in a false positive NameConflict error.

Should we reset the non-equivalence relation after a decl is imported for this 
decl and its redecls?

> By this change the error had gone, and we did not recognize any analysis 
> slowdown. Remember, we still have a cache, but not a global cache in the 
> ASTImporter object.

Hm, I wonder if our cache is really useful or not. Unfortunately, I never did 
any measures.

Still the lack of a test disturbs me, even despite the fact that I got the idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62131



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

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks for the fixes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62329



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

[PATCH] D63603: [ASTImporter] Propagate error from ImportDeclContext

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Gabor,
The patch looks good. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63603



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

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thanks for the explanation!
It will be good if someone else takes a look at this patch.




Comment at: clang/include/clang/AST/ASTImporterSharedState.h:40
+  /// never cleared (like ImportedFromDecls).
+  llvm::DenseMap ImportErrors;
+

ErrorsInToContext?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376



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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
Thank you for the explanation. I got the idea of this patch anyway, but it will 
be definitely useful for anyone digging into the code. Should we make it a 
comment for ImportPathTy?




Comment at: clang/lib/AST/ASTImporter.cpp:8682
+
+void ASTImporter::ImportPathTy::push(Decl *D) {
+  Nodes.push_back(D);

I think these definitions are small enough to move them into the class 
definitions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62375



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

[PATCH] D62373: [ASTImporter] Store import errors for Decls

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4697
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,

#undef ERRONEOUSSTMT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62373



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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62375



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

[PATCH] D62557: [analyzer] Modernize CStringChecker to use CallDescriptions.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D62557



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

[PATCH] D62556: [analyzer] NFC: CallDescription: Implement describing C library functions.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Artem,
Looks mostly good, but I have some comments inline.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1064
   // e.g. "{a, b}" represent the qualified names, like "a::b".
   std::vector QualifiedName;
   unsigned RequiredArgs;

Not for this review, but why do we have a vector of `const char *`, not 
StringRefs?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1066
   unsigned RequiredArgs;
+  int Flags;
 

Is it a good idea to make Flags a bitfield structure?



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:379
+
   if (!II || II != CD.II)
 return false;

`!II` is never false due to the newly-introduced early return.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62556



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

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor!
I haven't find the import sequence examples we try to fix these ways in any of 
the three patches these change consists of. Could you please provide some (or 
point if I missed them)?




Comment at: clang/include/clang/AST/ASTImporterSharedState.h:46
+public:
+  ASTImporterSharedState() {}
+

`= default?`



Comment at: clang/lib/AST/ASTImporter.cpp:7724
 void ASTImporter::AddToLookupTable(Decl *ToD) {
-  if (LookupTable)
+  if (SharedState->getLookupTable())
 if (auto *ToND = dyn_cast(ToD))

```
if (auto* LookupTable = ..._)
  ...
LookupTable->add();
```
looks a bit cleaner to me.



Comment at: clang/lib/AST/ASTImporter.cpp:7830
   if (ToD) {
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))

I'm not sure if it is safe to compare declarations from different TUs by 
pointers because they belong to different allocators.



Comment at: clang/lib/AST/ASTImporter.cpp:7831
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+  return make_error(*Error);

getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it 
intentional?



Comment at: clang/lib/AST/ASTImporter.cpp:7867
   if (PosF != ImportedFromDecls.end()) {
-if (LookupTable)
+if (SharedState->getLookupTable())
   if (auto *ToND = dyn_cast(ToD))

I think we can encapsulate these conditions into 
`SharedState::[add/remove]Decl[To/From]Lookup methods.



Comment at: clang/lib/AST/ASTImporter.cpp:7924
+  if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+return make_error(*Error);
+

I don' think that an import failure from another TU means that the import from 
the current TU will fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376



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

[PATCH] D62375: [ASTImporter] Mark erroneous nodes in from ctx

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Could you please provide any test for the import itself?




Comment at: clang/lib/AST/ASTImporter.cpp:8668
+
+bool ASTImporter::ImportPathTy::hasCycleAtBack() {
+  return Aux[Nodes.back()] > 1;

const?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62375



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

[PATCH] D62373: [ASTImporter] Store import errors for Decls

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
The idea looks fine to me, but I have some questions inline.




Comment at: clang/lib/AST/ASTImporter.cpp:5109
 } else { // ODR violation.
   // FIXME HandleNameConflict
+  return make_error(ImportError::NameConflict);

Is this FIXME obsolete now?



Comment at: clang/lib/AST/ASTImporter.cpp:7823
+auto Pos = ImportedDecls.find(FromD);
+if (Pos != ImportedDecls.end()) {
+  // Import failed after the object was created.

I see the enabled test case, but does it cover the logic in this block?



Comment at: clang/lib/AST/ASTImporter.cpp:7851
+if (!getImportDeclErrorIfAny(FromD)) {
+  // Error encountered for the first time.
+  // After takeError the error is not usable any more in ToDOrErr.

Is it possible to get this error more than once?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62373



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

[PATCH] D62440: [analyzer] NFC: Change evalCall() to provide a CallEvent.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:372
   CheckerContext &C) const {
-  const FunctionDecl *FD = dyn_cast_or_null(CE->getCalleeDecl());
+  const auto *FD = dyn_cast_or_null(Call.getDecl());
   if (!FD)

Should we create helpers similar to getDecl() and getOriginExpr 
(getFunctionDecl/getCallExpr)?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62440



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

[PATCH] D62329: [ASTImporter] Structural eq: handle DependentScopeDeclRefExpr

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
I have a few questions inline.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:124
+  case DeclarationName::CXXConversionFunctionName:
+return true;
+

Should we check the equivalence of getCXXNameType() in such cases?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:127
+  case DeclarationName::CXXDeductionGuideName:
+return IsStructurallyEquivalent(
+Name1.getCXXDeductionGuideTemplate()->getDeclName(),

Should we check the equivalence of the whole 
Name1.getCXXDeductionGuideTemplate() (with the template arguments)?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:147
+
+  return true;
+}

llvm_unreachable()?



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:163
+return IsStructurallyEquivalent(Context, DE1->getQualifier(),
+DE2->getQualifier());
+  }

Should we compare TemplateArgs (getTemplateArgs) somehow?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62329



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

[PATCH] D62064: [ASTImporter] Fix unhandled cases in ASTImporterLookupTable

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4259
 static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) {
   QualType Ty = FD->getFriendType()->getType();
+  if (auto *Inner = dyn_cast(Ty.getTypePtr())) {

martong wrote:
> a_sidorin wrote:
> > Will getCanonicalType() do the job?
> That's a good catch! Thank you for pointing out.
Can we replace the whole function with:
```
QualType Ty = FD->getFriendType()->getType().getCanonicalType();
return cast(Ty)->getDecl();
```
?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:4249
 
 static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) {
   QualType Ty = FD->getFriendType()->getType();

Redundant space after '*'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62064



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

[PATCH] D62131: [ASTImporter] Remove NonEquivalentDecls from ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Could you provide an example of an import sequence leading to this behavior? 
It's hard for me to imagine such a situation.




Comment at: clang/test/ASTMerge/struct/test.c:37
 // CHECK: struct2.c:53:37: note: field 'f' has type 'float' here
-// CHECK: struct1.c:54:8: warning: type 'struct DeepError' has incompatible 
definitions in different translation units
-// CHECK: struct1.c:56:41: note: field 'Deeper' has type 'struct DeeperError 
*' here

It looks strange to me that these warnings are gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62131



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

[PATCH] D62066: [ASTImporter] Enable disabled but passing tests

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Cool!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62066



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

[PATCH] D62064: [ASTImporter] Fix unhandled cases in ASTImporterLookupTable

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
This looks fine, but I have a question inline.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:4259
 static const RecordDecl * getRecordDeclOfFriend(FriendDecl *FD) {
   QualType Ty = FD->getFriendType()->getType();
+  if (auto *Inner = dyn_cast(Ty.getTypePtr())) {

Will getCanonicalType() do the job?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62064



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

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: clang/lib/Analysis/ReachableCode.cpp:465
+  CFGTerminator T = Block->getTerminator();
+  if (T.getKind() == CFGTerminator::StmtBranch) {
+const Stmt *S = T.getStmt();

isStmtBranch()?


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

https://reviews.llvm.org/D61814



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

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

The conversion operator indeed looks non-evident.




Comment at: clang/include/clang/Analysis/CFG.h:513
 public:
-  CFGTerminator() = default;
-  CFGTerminator(Stmt *S, bool TemporaryDtorsBranch = false)
-  : Data(S, TemporaryDtorsBranch) {}
+  CFGTerminator() { assert(!isValid()); }
+  CFGTerminator(Stmt *S, Kind K = StmtBranch) : Data(S, K) {}

It seems to me that `isStmt()` and `isTemporaryDtorBranch()` methods can make 
the code a bit cleaner in few places by avoiding comparisons with enums.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61814



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

[PATCH] D61815: [CFG] NFC: Modernize test/Analysis/initializers-cfg-output.cpp.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Wow, this is a cool idea!


Repository:
  rC Clang

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

https://reviews.llvm.org/D61815



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Thank you for digging into this! Feel free to ask me if you encounter any 
problems with the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D61786: [ASTImporter] Separate unittest files

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Gabor,
I like this change! LGTM, just a few nits.




Comment at: clang/unittests/AST/ASTImporterGenericRedeclTest.cpp:20
+
+// FIXME put these structs and the tests rely on them into their own separate
+// test file!

Is this FIXME obsolete now?



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:37
+// FunctionDecl:
+auto *ExternF = "void f();";
+auto *StaticF = "static void f();";

Can we use StringRef (or, at least `const auto *`)?



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:49
+::testing::WithParamInterface>;
+// Fixture to test the redecl chain of Decls with the same visibility.  Gtest
+// makes it possible to have either value-parameterized or type-parameterized

Double spaces in the end of sentences look a bit strange.



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:139
+
+auto *ToF0 = FirstDeclMatcher().match(ToTu, getPattern());
+auto *FromF1 = FirstDeclMatcher().match(FromTu, getPattern());

Optional: F0/F1 (where F stands for "function", I guess) can be replaced with 
D0/D1 (for decl) since the code is generic.



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:157
+TranslationUnitDecl *FromTu1 = getTuDecl(getCode1(), Lang_CXX, 
"input1.cc");
+auto *FromF0 =
+FirstDeclMatcher().match(FromTu0, getPattern());

This should fit a single line.



Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:190
+
+bool ExpectLink = true;
+bool ExpectNotLink = false;

const?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61786



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

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Aleksei Sidorin via Phabricator via cfe-commits]

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

👍




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:277
   merger.RemoveSources(importer_source);
-  return ret;
+  if (ret_or_error)
+return *ret_or_error;

'true' body needs to be surrounded by braces as well. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D61424: [ASTImporter] Fix inequivalence of unresolved exception spec

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Looks good!




Comment at: clang/lib/AST/ASTImporter.cpp:3107
+// noexcept-unevaluated, while the newly imported function may have an
+// evaluated noexcept.
   }

This looks to be true -  a call `adjustExceptionSpec()` on the imported decl 
and its redeclarations can be required. Thank you for noticing this!



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:325
 
+/// Check the eqeuivalence of exception specifications.
+static bool IsEquivalentExceptionSpec(StructuralEquivalenceContext &Context,

equivalence


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61424



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59692



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

[PATCH] D61333: [ASTImporter] Fix LLDB lookup in transparent ctx and with ext src

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hello Gabor!
This looks good to me, but let's wait for LLDB guys to take a look at the 
patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61333



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

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Looks good, thanks!


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

https://reviews.llvm.org/D61140



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

[PATCH] D61140: Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Shafik!
The patch itself is fine, but, as other reviewers pointed, tests are 
appreciated. I suggest to add a test into ASTImporterTests.cpp - you will find 
several ways to write tests of different complexity here. I think this change 
can be tested even with the simplest testImport() facility.


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

https://reviews.llvm.org/D61140



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

> Mmm, what are the pros and cons?

This is how we do it in the ASTImporter. It allows us to correctly handle 
redeclarations with and without bodies - the declarations in the current AST 
remain unchanged but a new declaration appears.  But it can be a kind of a 
professional deformation :) BodyFarm is a much lighter facility and I won't 
argue that my approach is better - just an idea.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Artem,
This looks good to me. However, it seems to me that the correct solution is to 
synthesize a shining new function and include it into the redeclaration chain. 
This requires much more work, however.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

I like the test even more than the change itself!




Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:49
+// Bind(Zero)
+Store StX0 =
+StMgr.Bind(StInit, LX0, Zero).getStore();

This can fit one line.



Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:82
+  VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+  ~VariableBindConsumer() override {}
+

Do we need this dtor declaration?



Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:93
+public:
+  VariableBindAction() {}
+  std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,

`= default`? (Or it seems like we can remove it at all)


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

https://reviews.llvm.org/D60742



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

[PATCH] D60739: [analyzer] NFC: Re-use reusable unittest mocks.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60739



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

[PATCH] D55049: Changed every use of ASTImporter::Import to Import_New

[Aleksei Sidorin via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55049



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

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hello Raphael,
I think we should accept this change. I don't see an easy way to merge the LLDB 
stuff into ASTImporter; also, this patch provides a good extension point for 
ASTImporter since it is designed to be a parent class. @martong @shafik Gabor, 
Shafik, what do you think?


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

https://reviews.llvm.org/D59485



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

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Yes, I think this is fine. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59761



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Thanks!


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

https://reviews.llvm.org/D59665



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Post-LGTM with some stylish nits.




Comment at: cfe/trunk/lib/AST/ASTImporter.cpp:1950
+  // Eliminate a potential failure point where we attempt to re-import
+  // something we're trying to import while completin ToEnum
+  if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum))

`completin_g_`; also, comments are required to be ended with dot.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59845



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Shafik,
Thank you for the explanation, it is much more clear to me now. But, as I see, 
D59692  is going to discard the changes this 
patch introduces. @martong Gabor, do you expect the changes of this patch to be 
merged into yours, or should this patch be abandoned?


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

https://reviews.llvm.org/D59665



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Thanks for the fixes! I'd like to clarify one moment, however.




Comment at: lib/AST/ASTImporter.cpp:2154
+return NameOrErr.takeError();
 }
   }

Should we write `else Name = *NameOrError`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59692



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

[PATCH] D55049: Changed every use of ASTImporter::Import to Import_New

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Balazs,

The looks mostly good to me.




Comment at: lib/AST/ASTImporter.cpp:3440
 
-  for (const auto *Attr : D->attrs())
-ToIndirectField->addAttr(Importer.Import(Attr));

There is the same deletion in D53757.



Comment at: lib/AST/ASTImporter.cpp:8550
+if (ExpectedType ToFromOrErr = Import_New(From)) {
+  if (ToContext.hasSameType(*ToFromOrErr, To))
+return true;

Wow, we import types instead of just checking them for structural equivalence. 
That's OK to leave it in the patch as-is but looks pretty strange. Maybe this 
even deserves a FIXME.



Comment at: unittests/AST/ASTImporterTest.cpp:146
+ << "Import failed, error: \"" << Err << "\"!";
+  llvm::consumeError(std::move(Err));
+}

Dead code?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55049



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

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Balasz,

Sorry, I missed the review accidentally. Thank you for the patch!




Comment at: lib/AST/ASTImporter.cpp:3418
 
-  for (const auto *Attr : D->attrs())
-ToIndirectField->addAttr(Importer.Import(Attr));

shafik wrote:
> Why is this section of code removed?
I guess the reason is that this import is already done inside 
`GetImportedOrCreateDecl()`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53757



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

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Raphael,

Thank you for the explanation. I have +1 to Gabor's question to understand if 
this functionality can actually be added to the common ASTImporter.




Comment at: clang/include/clang/AST/ASTImporter.h:149
+/// decl on its own.
+virtual Expected ImportInternal(Decl *From);
+

I'd suggest to rename it to `ImportImpl`.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

LookupTable?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:578
+auto *ND = dyn_cast(FromD);
+if (!ND)
+  return ASTImporter::ImportInternal(FromD);

I think it's better to merge these conditions: `if (!ND || ND->getName() != 
"shouldNotBeImported")`



Comment at: clang/unittests/AST/ASTImporterTest.cpp:583
+for (Decl *D : getToContext().getTranslationUnitDecl()->decls()) {
+  if (auto ND = dyn_cast(D))
+if (ND->getName() == "realDecl")

`auto *` (pointer sign is needed).


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

https://reviews.llvm.org/D59485



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

Thank you for addressing the problem!




Comment at: lib/AST/ASTImporter.cpp:2256
 return Importer.MapImported(D, FoundTypedef);
-}
-// FIXME Handle redecl chain.
-break;
+} else
+  ConflictingDecls.push_back(FoundDecl);

`if` body is surrounded by braces, so it's better to surround `else` too.



Comment at: lib/AST/ASTImporter.cpp:2260
 
   ConflictingDecls.push_back(FoundDecl);
 }

Do we push the same decl twice?



Comment at: lib/AST/ASTImporter.cpp:8532
 unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
 }

Empty DeclarationName can be valid sometimes. Should we return 
ErrorOr instead? This can also simplify caller code a bit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59692



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a subscriber: davide.
a_sidorin added a comment.

Hi Shafik,

Honestly, I was always wondering what does HandleNameConflict actually do. Its 
implementation in ASTImporter is trivial and I don't see any of its overrides 
in LLDB code too. Why do we check its result to be non-empty is a question to 
me as well. And the more I look at it (and the bug you pointed in it), the more 
I think there is something wrong with it. Maybe it is better to just remove it 
at all? I hope LLDB developers have more knowledge about it. Shafik, Davide 
@davide , do you know its actual purpose?




Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

If I understand correctly, this will replace Name with SearchName causing an 
anonymous enum to be imported as a named a few lines below. It doesn't look 
like a correct behaviour to me.


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

https://reviews.llvm.org/D59665



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

[PATCH] D58897: [ASTImporter] Make ODR error handling configurable

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
This patch LGTM mostly, but there is a comment inline.




Comment at: include/clang/AST/ASTStructuralEquivalence.h:81
 EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling),
-ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain) {}
+Complain(Complain), ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch) {}
 

I see the argument order change but I don't see any callers changed. Do we 
really need this order change?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58897



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D55358: [ASTImporter] Fix import of NestedNameSpecifierLoc.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Looks good!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55358



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

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Gabor,
Thanks for the patch! It looks good to me except some stylish nits.




Comment at: lib/AST/ASTImporter.cpp:5130
+Importer.MapImported(D, PrevDecl->getDefinition());
+// Import those those default field initializers which have been
+// instantiated in the "From" context, but not in the "To" context.

Could you please also replace "those those" to "those" in the comment below?



Comment at: lib/AST/ASTImporter.cpp:5144
+// Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
+// what
+// else could be fused during an AST merge.

The comment is a bit broken.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
The patch looks almost good bu I have some comments inline.




Comment at: lib/AST/ASTImporter.cpp:3002
 
+  // Check if we have found an existing definition.  Returns with that
+  // definition if yes, otherwise returns null.

I like this lambda. To make the code even better, we can move this lambda 
outside of VisitFunctionDecl because this method is already pretty big.



Comment at: unittests/AST/ASTImporterTest.cpp:3862
 
+  void CheckPreviousDecl(Decl *To0, Decl *To1) {
+ASSERT_NE(To0, To1);

I don't like numbers. Maybe `To0` and `To1` are `LastDecl` and `ImportedDecl`, 
correspondingly?



Comment at: unittests/AST/ASTImporterTest.cpp:3881
+if (auto *From0F = dyn_cast(To0)) {
+  auto *To0F = cast(To0);
+  if (From0F->getTemplatedKind() ==

To0 and From0F actually have the same value, and To0F is unused.



Comment at: unittests/AST/ASTImporterTest.cpp:3884
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+EXPECT_EQ(To0->getCanonicalDecl(), To1->getCanonicalDecl());
+// There may be a hidden fwd spec decl before a spec decl.

If I don't miss something, this assumption does not depend on To0 and To1 kind 
and can be moved out of the condition, to the function scope.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thanks for the fixes!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D57590: [ASTImporter] Improve import of FileID.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Sorry, missed the patch. It looks mostly good, but do we have any test for this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57590



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
The patch looks OK overall but I have some comments inline.




Comment at: lib/AST/ASTImporter.cpp:4966
 // it has any definition in the redecl chain.
-static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
-  CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+template  static auto getDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();

We should point that this function is for TemplateDecls only somehow. But we 
can't just pass TemplateDecl as the parameter due to loss of the actual return 
type. Maybewe should rename this function into "getTemplateDefinition()"?



Comment at: lib/AST/ASTImporter.cpp:5563
+  // TODO: handle conflicting names
+} // linkage
+  }   // template

We don't usually put such comments after control flow statements. If they are 
really needed, it is a good sign that a function must be split, and it's better 
to leave a FIXME for this (or do the split).


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Gabor,
I don't see any problems with the patch. Thanks! I think it will be good to get 
Shafik's approval as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58502



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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Tom,
Thanks for the fixes! The patch looks good to me now. I have only a small nit 
inline.




Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

aaron.ballman wrote:
> aaron.ballman wrote:
> > a_sidorin wrote:
> > > aaron.ballman wrote:
> > > > Please don't use `auto` here; the type isn't spelled out in the 
> > > > initialization.
> > > Hi Aaron,
> > > importSeq() is a helper targeted to simplify a lot of imports done during 
> > > AST node import and related error handling. The type of this 
> > > `importSeq()` expression is `Expected > > SourceLocation, SourceLocation, QualType>>`, so I think it's better to 
> > > leave it as `auto`.
> > Wow. I really dislike that we basically *have* to hide the type information 
> > because it's just too ugly to spell. I understand why we're using `auto` 
> > based on your explanation, but it basically means I can't understand what 
> > this code is doing without a lot more effort.
> > 
> > Let's keep the `auto`, but let's please stop using type compositions that 
> > make it considerably harder to review the code. This feel like a misuse of 
> > `std::tuple`.
> After staring at this for longer, I no longer think it's a misuse of 
> `std::tuple`, just... an unfortunate side-effect of the design of 
> `importSeq()`. I don't have a good idea for how to make this easier on 
> reviewers and other people who aren't frequently looking at the AST importing 
> code. :-(
I think it is a case where the type is not even important. With C++17, we would 
just write:
```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
if (Imp)
  return Imp.takeError();
auto [ToCond, ToLHS, ToRHS] = *Imp;
```
The exact type is not really needed here. However, if you have any ideas on how 
to improve this and make the type more explicit - they are always welcome.



Comment at: unittests/AST/ASTImporterTest.cpp:581
+  // Don't try to match the template contents if template parsing is delayed.
+  if (std::find(std::begin(Args), std::end(Args),
+"-fdelayed-template-parsing") != Args.end()) {

LLVm has a set of nice helpers for working with containers in range -like 
style. I'd recommend you to use llvm::find here:
`if (llvm::find(Args), "-fdelayed-template-parsing") != Args.end()) { `


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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

[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

aaron.ballman wrote:
> tmroeder wrote:
> > aaron.ballman wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > E->isConditionTrue();`
> > > > `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
> > > I like that even better than my suggestion. :-)
> > Wait, this doesn't have the same truth table as my original code.
> > 
> > let `CD = E->isConditionDependent()`
> > let `CT = E->isConditionTrue()`
> > 
> > in
> > 
> > ```
> > bool CondIsTrue = false;
> > if (!CD)
> >   CondIsTrue = CT;
> > ```
> > 
> > has the table for `CondIsTrue`:
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | F |
> > | T | F | F |
> > | F | T | T |
> > | F | F | F |
> > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> > value of CT.
> > 
> > The suggested line has the truth table
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | T |
> > | T | F | F |
> > | F | T | F |
> > | F | F | F |
> > 
> > That is, the effect of CD is swapped.
> > 
> > Aaron's suggestion matches my original table. I based my code on 
> > include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() 
> > in the implementation of isConditionTrue.
> > 
> > I realized this after I "fixed" my comment to match the implementation 
> > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > think this should be
> > 
> > ```
> > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > ```
> > 
> > I've changed my code to that and reverted the comment change.
> Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> should add a test case that exercises this?
Wow, that's a nice catch. Sorry for the misleading.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58292



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