MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Driver/ToolChains/Arch/Sparc.cpp:121
+ StringRef CPUName = A->getValue();
+
+ if (CPUName == "native") {
----------------
delete blank line
================
Comment at: clang/lib/Driver/ToolChains/Arch/Sparc.cpp:135
+ return "v9";
+ else
+ return "";
----------------
The common style omits `else` here
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2226
+
+ std::string TuneCPU;
+ if (Name == "native")
----------------
ro wrote:
> MaskRay wrote:
> > ```
> > std::string TuneCPU(Name == "native" ? ... : ...)
> > if (!TuneCPU.empty()) {
> > ...
> > ```
> I'm not sure about this: I tried that variant, but I don't really think it's
> clearer than what I have now:
> ```
> std::string TuneCPU(Name == "native"
> ? std::string(llvm::sys::getHostCPUName()
> : std::string(Name)));
> ```
> My code was taken from `AddSystemZTargetArgs` directly below and it would
> seem a bit weird if they differ in style.
OK, but I think AddSystemZTargetArgs somewhat deviates from common styles.
Since `llvm::sys::getHostCPUName()` cannot be empty, and `-mtune=` (empty
value) should be an error (but only aarch64 seems to emit an error), I'd omit
the `if (!TuneCPU.empty()) ` test. For aarch64, I made such a simplification:
475e526d85003404ba521e15f8acef1b439fb910
I don't mind whether sparc emits an error for `-mtune=` or not.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130273/new/
https://reviews.llvm.org/D130273
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
