aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2389-2397
def warn_unsupported_target_attribute
: Warning<"Ignoring unsupported '%0' in the target attribute string">,
InGroup<IgnoredAttributes>;
+def warn_duplicate_target_attribute
+ : Warning<"Ignoring duplicate '%0' in the target attribute string">,
+ InGroup<IgnoredAttributes>;
+def warn_unsupported_target_architecture
----------------
All of these (including the one you didn't have to touch) should be using
"ignoring" instead of "Ignoring".
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2395
+ InGroup<IgnoredAttributes>;
+def warn_unsupported_target_architecture
+ : Warning<"Ignoring unsupported architecture '%0' in the target attribute "
----------------
This name should have something to do with attributes. Might make sense to
combine with `warn_unsupported_target_attribute` using a `%select` given the
similarity in use case.
================
Comment at: lib/Basic/Targets.cpp:1015-1018
+ // Note: GCC recognizes the following additional cpus:
+ // 401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
+ // 821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
+ // titan, rs64.
----------------
I think this comment should be hoisted up to `isValidCPUName()`.
================
Comment at: lib/Basic/Targets.cpp:6416
+ bool setCPU(const std::string &Name) override {
+ return isValidCPUName(Name);
+ }
----------------
It's rather strange that this doesn't actually set the CPU, but I can't see
that the original code did either, so maybe it's fine?
================
Comment at: lib/Basic/Targets.cpp:9617
+ bool isValid = isValidCPUName(Name);
+ if (isValid) this->CPU = Name;
+ return isValid;
----------------
You can drop the `this`, and I would put the `CPU = Name` onto its own line
(not certain if that's a personal preference or a general community style).
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3004
+ TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
+ if(ParsedAttrs.DuplicateArchitecture)
+ return Diag(LiteralLoc, diag::warn_duplicate_target_attribute) << "arch";
----------------
Missing a space after the `if`.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3005
+ if(ParsedAttrs.DuplicateArchitecture)
+ return Diag(LiteralLoc, diag::warn_duplicate_target_attribute) << "arch";
+
----------------
Should this be `"arch="` to be consistent with the previous diagnostic on line
3001?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3007
+
+ if (!ParsedAttrs.Architecture.empty() &&
+ !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
----------------
Is the attribute valid when the architecture is empty?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3012
+
+ for (auto &Feature : ParsedAttrs.Features) {
+ auto CurFeature = StringRef(Feature).drop_front();// remove + or -.
----------------
Can this be made `const auto &`?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3013
+ for (auto &Feature : ParsedAttrs.Features) {
+ auto CurFeature = StringRef(Feature).drop_front();// remove + or -.
+ if (!Context.getTargetInfo().isValidFeatureName(CurFeature))
----------------
Insert a space before the comment.
================
Comment at: test/Sema/attr-target.c:9
+int __attribute__((target("woof"))) bark() { return 4; }//expected-warning
{{Ignoring unsupported 'woof' in the target attribute string}}
+
----------------
Please add a test case where there's a duplicate arch and an empty arch.
https://reviews.llvm.org/D35519
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
