[clang][ASTImporter] Simplify code of attribute import [NFC].

[balazske]

Summary

The code of ASTImporter::Import(const Attr *) was repetitive,
it is now simplified. (There is still room for improvement but
probably only after big changes.)

[martong]

Awesome! Thanks!

Please indicate in the title that this is an [NFC].

[shafik]

clang/lib/AST/ASTImporter.cpp

8516–8517

I am enthusiastic about reference members if we can somehow avoid them.

Can we use a struct to return both the Err and the pointer together? I don't like std::pair mainly b/c first and second are not meaningful.

[balazske]

AttrImporter owns now the result of import (until it is read),
instead of referenced error and returned pointer.

[balazske]

Update the code comments.

[steakhal]

clang/lib/AST/ASTImporter.cpp

8585–8586

If it can be used only once, we should mark this function as an r-value function.
There is a similar macro already defined as LLVM_LVALUE_FUNCTION.

Later, when you would actually call this function, you need to std::move() the object, signifying that the original object gets destroyed in the process.

@aaron.ballman Do you think we need to define LLVM_RVALUE_FUNCTION or we can simply use the && in the function declaration?
I've seen that you tried to substitute all LLVM_LVALUE_FUNCTION macros in the past. What's the status on this?

[aaron.ballman]

clang/lib/AST/ASTImporter.cpp

8585–8586

The expected pattern is:

#if LLVM_HAS_RVALUE_REFERENCE_THIS
  void func() && {
  }
#endif

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchersInternal.h#L609 (and elsewhere). However, I note that there are some unprotected uses (such as https://github.com/llvm/llvm-project/blob/main/clang/lib/Tooling/Syntax/BuildTree.cpp#L437) and so it may be a sign that we're fine to remove LLVM_HAS_RVALUE_REFERENCE_THIS because all our supported compilers do the right thing?

[steakhal]

clang/lib/AST/ASTImporter.cpp

8585–8586

You tried to eliminate that on Jan 22, 2020 with rGdfe9f130e07c929d21f8122272077495de531a38.
But according to git blame, the BuildTree.cpp#L437 was committed on Jul 9, 2019 with rG9b3f38f99085e2bbf9db01bb00d4c6d837f0fc00.
I'm confused.

[aaron.ballman]

clang/lib/AST/ASTImporter.cpp

8585–8586

I'm confused.

As am I. I went back and looked at the history here, and as best I can tell, I tried to get rid of lvalue functions, we threw it at bots, and for reasons I didn't capture anywhere I can find, had to revert it. Sorry for the troubles with not tracking that information! :-(

However, in my simple testing on godbolt, I can't find a version of MSVC that has issues with lvalue or rvalue reference overloads. We claim to still support MSVC 2017 (perhaps it's time to bump that to something more recent now that MSVC has changed its release schedule), so maybe there's an older version that's still an issue, but I would expect us to have spotted that given there are unprotected uses of rvalue ref overloads.

My gut feeling is that we should explore getting rid of LLVM_LVALUE_FUNCTION and LLVM_HAS_RVALUE_REFERENCE_THIS again as it's been almost two years since the last try. If there's a problematic version of MSVC, we might want to consider dropping support for it unless it persists in newer MSVC versions.

[balazske]

Adding && and new asserts to use AttrImporter only once.

[steakhal]

I think it looks great.
Thank you for addressing this.

clang/lib/AST/ASTImporter.cpp

template <typename T> struct AttrArgArrayImporter {
 
   T *value() { return To.data(); }
 
 private:
   llvm::SmallVector<T, 2> To;
 };
 
 class AttrImporter {
-  Error Err = Error::success();
+  Error Err{Error::success()};
+  Attr *ToAttr = nullptr;
   ASTImporter &Importer;
   ASTNodeImporter NImporter;
 
 public:
   AttrImporter(ASTImporter &I) : Importer(I), NImporter(I) {}
 
   // Create an "importer" for an attribute parameter.
   // Result of the 'value()' of that object is to be passed to the function
-  // 'createImpoprtedAttr', in the order that is expected by the attribute
-  // class.
+  // 'importAttr', in the order that is expected by the attribute class.
   template <class T> AttrArgImporter<T> importArg(const T &From) {
     return AttrArgImporter<T>(NImporter, Err, From);
   }
 
   // Create an "importer" for an attribute parameter that has array type.
   // Result of the 'value()' of that object is to be passed to the function
-  // 'createImpoprtedAttr', then the size of the array as next argument.
+  // 'importAttr', then the size of the array as next argument.
   template <typename T>
   AttrArgArrayImporter<T> importArrayArg(const llvm::iterator_range<T *> &From,
                                          unsigned ArraySize) {
     return AttrArgArrayImporter<T>(NImporter, Err, From, ArraySize);
   }
 
   // Create an attribute object with the specified arguments.
   // The 'FromAttr' is the original (not imported) attribute, the 'ImportedArg'
   // should be values that are passed to the 'Create' function of the attribute.
   // (The 'Create' with 'ASTContext' first and 'AttributeCommonInfo' last is
   // used here.) As much data is copied or imported from the old attribute
   // as possible. The passed arguments should be already imported.
+  // If an import error happens, the internal error is set to it, and any
+  // further import attempt is ignored.
   template <typename T, typename... Arg>
-  Expected<Attr *> createImportedAttr(const T *FromAttr, Arg &&...ImportedArg) {
+  void importAttr(const T *FromAttr, Arg &&...ImportedArg) {
     static_assert(std::is_base_of<Attr, T>::value,
                   "T should be subclass of Attr.");
+    assert(!ToAttr && "Use one AttrImporter to import one Attribute object.");
 
     const IdentifierInfo *ToAttrName = Importer.Import(FromAttr->getAttrName());
     const IdentifierInfo *ToScopeName =
         Importer.Import(FromAttr->getScopeName());
     SourceRange ToAttrRange =
         NImporter.importChecked(Err, FromAttr->getRange());
     SourceLocation ToScopeLoc =
         NImporter.importChecked(Err, FromAttr->getScopeLoc());
 
     if (Err)
-      return std::move(Err);
+      return;
 
     AttributeCommonInfo ToI(ToAttrName, ToScopeName, ToAttrRange, ToScopeLoc,
                             FromAttr->getParsedKind(), FromAttr->getSyntax(),
                             FromAttr->getAttributeSpellingListIndex());
     // The "SemanticSpelling" is not needed to be passed to the constructor.
     // That value is recalculated from the SpellingListIndex if needed.
-    T *ToAttr = T::Create(Importer.getToContext(),
-                          std::forward<Arg>(ImportedArg)..., ToI);
+    ToAttr = T::Create(Importer.getToContext(),
 
     ToAttr->setImplicit(FromAttr->isImplicit());
     ToAttr->setPackExpansion(FromAttr->isPackExpansion());
     if (auto *ToInheritableAttr = dyn_cast<InheritableAttr>(ToAttr))
       ToInheritableAttr->setInherited(FromAttr->isInherited());
+  }
+
+  // Create a clone of the 'FromAttr' and import its source range only.
+  // This causes objects with invalid references to be created if the 'FromAttr'
+  // contains other data that should be imported.
+  void cloneAttr(const Attr *FromAttr) {
+    assert(!ToAttr && "Use one AttrImporter to import one Attribute object.");
 
+    SourceRange ToRange = NImporter.importChecked(Err, FromAttr->getRange());
+    if (Err)
+      return;
+
+    ToAttr = FromAttr->clone(Importer.getToContext());
+    ToAttr->setRange(ToRange);
+  }
+
+  // Get the result of the previous import attempt (can be used only once).
+  llvm::Expected<Attr *> getResult() && {
+    if (Err)
+      return std::move(Err);
+    assert(ToAttr && "Attribute should be created.");
     return ToAttr;
   }
 };
 
 Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) {
-  Attr *ToAttr = nullptr;
-  // FIXME: Use AttrImporter as much as possible, try to remove the import
-  // of range from here.
-  SourceRange ToRange;
-  if (Error Err = importInto(ToRange, FromAttr->getRange()))
-    return std::move(Err);
+  AttrImporter AI(*this);
 
   // FIXME: Is there some kind of AttrVisitor to use here?
   switch (FromAttr->getKind()) {
   case attr::Aligned: {
     auto *From = cast<AlignedAttr>(FromAttr);
-    AlignedAttr *To;
-    auto CreateAlign = [&](bool IsAlignmentExpr, void *Alignment) {
-      return AlignedAttr::Create(ToContext, IsAlignmentExpr, Alignment, ToRange,
-                                 From->getSyntax(),
-                                 From->getSemanticSpelling());
-    };
-    if (From->isAlignmentExpr()) {
-      if (auto ToEOrErr = Import(From->getAlignmentExpr()))
-        To = CreateAlign(true, *ToEOrErr);
-      else
-        return ToEOrErr.takeError();
-    } else {
-      if (auto ToTOrErr = Import(From->getAlignmentType()))
-        To = CreateAlign(false, *ToTOrErr);
-      else
-        return ToTOrErr.takeError();
-    }
-    To->setInherited(From->isInherited());
-    To->setPackExpansion(From->isPackExpansion());
-    To->setImplicit(From->isImplicit());
-    ToAttr = To;
+    if (From->isAlignmentExpr())
+      AI.importAttr(From, true, AI.importArg(From->getAlignmentExpr()).value());
+      AI.importAttr(From, false,
+                    AI.importArg(From->getAlignmentType()).value());
     break;
   }
+
   case attr::Format: {
     const auto *From = cast<FormatAttr>(FromAttr);
-    FormatAttr *To;
-    IdentifierInfo *ToAttrType = Import(From->getType());
-    To = FormatAttr::Create(ToContext, ToAttrType, From->getFormatIdx(),
-                            From->getFirstArg(), ToRange, From->getSyntax());
-    To->setInherited(From->isInherited());
-    ToAttr = To;
+    AI.importAttr(From, Import(From->getType()), From->getFormatIdx(),
+                  From->getFirstArg());
     break;
   }
 
   case attr::AssertCapability: {
     const auto *From = cast<AssertCapabilityAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::AcquireCapability: {
     const auto *From = cast<AcquireCapabilityAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::TryAcquireCapability: {
     const auto *From = cast<TryAcquireCapabilityAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArg(From->getSuccessValue()).value(),
+    AI.importAttr(From, AI.importArg(From->getSuccessValue()).value(),
                   AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::ReleaseCapability: {
     const auto *From = cast<ReleaseCapabilityAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::RequiresCapability: {
     const auto *From = cast<RequiresCapabilityAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::GuardedBy: {
     const auto *From = cast<GuardedByAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr =
-        AI.createImportedAttr(From, AI.importArg(From->getArg()).value());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
+    AI.importAttr(From, AI.importArg(From->getArg()).value());
     break;
   }
   case attr::PtGuardedBy: {
     const auto *From = cast<PtGuardedByAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr =
-        AI.createImportedAttr(From, AI.importArg(From->getArg()).value());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
+    AI.importAttr(From, AI.importArg(From->getArg()).value());
     break;
   }
   case attr::AcquiredAfter: {
     const auto *From = cast<AcquiredAfterAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::AcquiredBefore: {
     const auto *From = cast<AcquiredBeforeAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::AssertExclusiveLock: {
     const auto *From = cast<AssertExclusiveLockAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::AssertSharedLock: {
     const auto *From = cast<AssertSharedLockAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::ExclusiveTrylockFunction: {
     const auto *From = cast<ExclusiveTrylockFunctionAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArg(From->getSuccessValue()).value(),
+    AI.importAttr(From, AI.importArg(From->getSuccessValue()).value(),
                   AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::SharedTrylockFunction: {
     const auto *From = cast<SharedTrylockFunctionAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArg(From->getSuccessValue()).value(),
+    AI.importAttr(From, AI.importArg(From->getSuccessValue()).value(),
                   AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
   case attr::LockReturned: {
     const auto *From = cast<LockReturnedAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr =
-        AI.createImportedAttr(From, AI.importArg(From->getArg()).value());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
+    AI.importAttr(From, AI.importArg(From->getArg()).value());
     break;
   }
   case attr::LocksExcluded: {
     const auto *From = cast<LocksExcludedAttr>(FromAttr);
-    AttrImporter AI(*this);
-    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
-        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+    AI.importAttr(From,
+                  AI.importArrayArg(From->args(), From->args_size()).value(),
                   From->args_size());
-    if (ToAttrOrErr)
-      ToAttr = *ToAttrOrErr;
-    else
-      return ToAttrOrErr.takeError();
     break;
   }
 
-  default:
-    // FIXME: 'clone' copies every member but some of them should be imported.
-    // Handle other Attrs that have parameters that should be imported.
-    ToAttr = FromAttr->clone(ToContext);
-    ToAttr->setRange(ToRange);
+  default: {
+    // The default branch works for attributes that have no arguments to import.
+    // FIXME: Handle every attribute type that has arguments of type to import
+    // (most often Expr* or Decl* or type) in the switch above.
+    AI.cloneAttr(FromAttr);
     break;
   }
-  assert(ToAttr && "Attribute should be created.");
-  
-  return ToAttr;
+  }
+  return std::move(AI).getResult();
 }
 
 Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
   auto Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end())
     return Pos->second;
   else
     return nullptr;