[PATCH] D30536: [tablegen][globalisel] Convert the SelectionDAG importer to a tree walking approach. NFC

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1281-1283
+  auto TrueOrError = importRulePredicates(M, P.getPredicates()->getValues());
+  if (auto Error = TrueOrError.takeError())
+    return std::move(Error);
----------------
ab wrote:
> Here and elsewhere: is it possible to fold the call into the if?  Something like:
> 
>   if (auto Err = importRulePredicates(M, P.getPredicates()->getValues()).takeError())
>     return Err;
> 
> That also lets you avoid the not-very-descriptive 'TrueOrError'.
> 
> More generally, an 'Expected<bool>' that's always true seems odd.  If you don't have plans to add more information, maybe just return an Error?  Then the whole takeError vs weird-name problem goes away.
> Here and elsewhere: is it possible to fold the call into the if? Something like:
>
> if (auto Err = importRulePredicates(M, P.getPredicates()->getValues()).takeError())
>   return Err;
> That also lets you avoid the not-very-descriptive 'TrueOrError'.

I'll make this change in the commit.

> More generally, an 'Expected<bool>' that's always true seems odd.

The bool is only there because Expected<void> isn't allowed. Optional<Error> is a bit more accurate but it has a different API and you then have to remember which functions return Expected<X> and which return Optional<Error>. I decided it was best to lean towards consistency.

> If you don't have plans to add more information, maybe just return an Error? Then the
> whole takeError vs weird-name problem goes away.

If a function always returns an Error, what does it return on success? It would be strange to have an Error that is not an error.


https://reviews.llvm.org/D30536