[PATCH] D27751: [LLVM] Use after move bug fixes

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek marked an inline comment as done.
Prazek added inline comments.


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:53
   if (E.isA<InstrProfError>()) {
     handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
       instrprof_error instrError = IPE.get();
----------------
Prazek wrote:
> Prazek wrote:
> > mehdi_amini wrote:
> > > mehdi_amini wrote:
> > > > vsk wrote:
> > > > > mehdi_amini wrote:
> > > > > > I'm wondering why there is a check for error before calling handleAllErrors here? (@lhames while you're at it...)
> > > > > If it is an instrprof error, we'd like the chance to inspect it and display an additional hint, instead of simply printing out a string representation of the error.
> > > > The check still does not seem idiomatic to me, it could be instead:
> > > > 
> > > > ```
> > > >     E = handleErrors(std::move(E), [&](const InstrProfError &IPE) {
> > > >      instrprof_error instrError = IPE.get();
> > > >       StringRef Hint = "";
> > > >       if (instrError == instrprof_error::unrecognized_format) {
> > > >         // Hint for common error of forgetting -sample for sample profiles.
> > > >         Hint = "Perhaps you forgot to use the -sample option?";
> > > >       }
> > > >       exitWithError(IPE.message(), Whence, Hint);
> > > >     });
> > > >     exitWithError(toString(std::move(E)), Whence);
> > > > ````
> > > I *think* you can also write:
> > > 
> > > ```
> > >     handleAllErrors(std::move(E),
> > >       [&](const InstrProfError &IPE) {
> > >        instrprof_error instrError = IPE.get();
> > >         StringRef Hint = "";
> > >         if (instrError == instrprof_error::unrecognized_format) {
> > >           // Hint for common error of forgetting -sample for sample profiles.
> > >           Hint = "Perhaps you forgot to use the -sample option?";
> > >         }
> > >         exitWithError(IPE.message(), Whence, Hint);
> > >       },
> > >       [&](const Error &E) {
> > >         exitWithError(toString(E), Whence);
> > >     });
> > > ````
> > Both seems fine, but I guess only the first one will get rid of the warning, because compiler will not be able to reason that handleErrors is noreturn because each lambda is noreturn
> oh ok I got it, we get rid of the second exitWithError call and use after move
ok, the code right now doesn't compile here, but I will fix it (toString takes E by value)


https://reviews.llvm.org/D27751