[PATCH] D68050: WIP Make attribute target work better with AArch64

[Nick Desaulniers via Phabricator via llvm-commits]

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

Thanks for the patch.  No comment if this is "the right thing to do," just basic code review comments.



================
Comment at: clang/include/clang/Basic/Attr.td:2107
     };
-    ParsedTargetAttr parse() const {
-      return parse(getFeaturesStr());
+    ParsedTargetAttr parse(StringRef SplitChars) const {
+      return parse(getFeaturesStr(), SplitChars);
----------------
C++ in a .td file? *mind blown*


================
Comment at: clang/include/clang/Basic/Attr.td:2126
 
     // Gets the list of features as simple string-refs with no +/- or 'no-'.
     // Only adds the items to 'Out' that are additions.
----------------
This comment is now obsoleted by this change, I think?


================
Comment at: clang/include/clang/Basic/Attr.td:2154
 
-    static ParsedTargetAttr parse(StringRef Features) {
+    static ParsedTargetAttr parse(StringRef Features, StringRef SplitChars) {
       ParsedTargetAttr Ret;
----------------
So many definitions of `parse`! If this one had `Features` be defaulted to `getFeaturesStr()`, then we could eliminate the definition on 2107.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:112
+StringRef AArch64TargetInfo::getTargetAttrSplitChars() const {
+  return ",+";
 }
----------------
I think it could be a minus sign, too?

`-mattr=-neon,-fp64,-d32`
`-mattr=+fullfp16,+thumb-mode`


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2976
+  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(
+      AttrStr, Context.getTargetInfo().getTargetAttrSplitChars());
 
----------------
It's too bad the `TargetAttr` has no notion of `TargetInfo`; it's kind of ugly to have to pass this in.


================
Comment at: llvm/include/llvm/ADT/StringRef.h:783-784
+    void split_on_one_of(SmallVectorImpl<StringRef> &A,
+               StringRef Separators, int MaxSplit = -1,
+               bool KeepEmpty = true) const;
+
----------------
So for `MaxSplit` and `KeepEmpty` I feel like YAGNI may apply here.  Default arguments that are never changed in the callers is a slight code smell.


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

https://reviews.llvm.org/D68050