[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Rafael! Please find my comments inline.



================
Comment at: lib/AST/ASTImporter.cpp:2650
+  for (const auto *A : D->attrs())
+    ToIndirectField->addAttr(Importer.Import(const_cast<Attr *>(A)));
 
----------------
Could we just remove 'const' qualifier from `A` to avoid `const_cast`? (Same below)


================
Comment at: lib/AST/ASTImporter.cpp:6547
+Attr *ASTImporter::Import(Attr *FromAttr) {
+  if (!FromAttr)
+    return nullptr;
----------------
Is it possible to get into a situation where a nullptr attribute is imported?


================
Comment at: lib/AST/ASTImporter.cpp:7211
     for (auto *FromAttr : From->getAttrs())
-      To->addAttr(FromAttr->clone(To->getASTContext()));
+      To->addAttr(Import(FromAttr));
   }
----------------
As I can see, `Import(Attr *)` can return nullptr. And I'm not sure that `addAttr(nullptr)` has well-defined behaviour.


================
Comment at: test/Import/attr/Inputs/S.cpp:4
+struct S
+{
+    struct {
----------------
Could you please clang-format the file?


https://reviews.llvm.org/D46115