[PATCH] D110810: [clang][ASTImporter] Simplify code of attribute import [NFC].

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8530-8531
 
+  // Get the result of the previous import attempt (can be used only once).
+  llvm::Expected<Attr *> getResult() {
+    if (Err)
----------------
steakhal wrote:
> aaron.ballman wrote:
> > steakhal wrote:
> > > 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?
> > 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?
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110810