[PATCH] D143645: [llvm] Fix MCSubtargetInfo::checkFeatures to handle unknown features correctly

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu marked 2 inline comments as done.
RamNalamothu added a comment.

Thanks for the comments @dblaikie @craig.topper.



================
Comment at: llvm/lib/MC/MCSubtargetInfo.cpp:81-87
+    return true;
   } else {
     errs() << "'" << Feature << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
+    return false;
   }
+  return false;
----------------
dblaikie wrote:
> avoid else-after-return (per LLVM coding conventions)
> 
> And it might even be worth inverting the previous condition (so it's `if (!FeatureEntry) { error stuff return false; } non-error stuff here`)
> 
> But also: we can't/don't usually print errors from arbitrary partts of LLVM - it's not use-LLVM-as-a-library friendly (the library client can't stop these messages from printing, render them in a different way/to a different mechanism (a log, a GUI, etc))
> 
> Are there other error handling conventions in use in nearby code you can find some inspiration from?
> avoid else-after-return (per LLVM coding conventions)
> 
> And it might even be worth inverting the previous condition (so it's `if (!FeatureEntry) { error stuff return false; } non-error stuff here`)
> 

Done, thank you.

> But also: we can't/don't usually print errors from arbitrary partts of LLVM - it's not use-LLVM-as-a-library friendly (the library client can't stop these messages from printing, render them in a different way/to a different mechanism (a log, a GUI, etc))
> 
> Are there other error handling conventions in use in nearby code you can find some inspiration from?

Since the entire `MCSubtargetInfo.cpp` is using this same error printing mechanism and I can't find a quick fix, I think that change can be deferred to another patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143645