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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
