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

[Erich Keane via Phabricator via llvm-commits]

erichkeane marked 4 inline comments as done.
erichkeane added inline comments.


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


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:112
+StringRef AArch64TargetInfo::getTargetAttrSplitChars() const {
+  return ",+";
 }
----------------
nickdesaulniers wrote:
> I think it could be a minus sign, too?
> 
> `-mattr=-neon,-fp64,-d32`
> `-mattr=+fullfp16,+thumb-mode`
attribute-target doesn't support 'minus'.  It is additive only.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2976
+  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(
+      AttrStr, Context.getTargetInfo().getTargetAttrSplitChars());
 
----------------
nickdesaulniers wrote:
> It's too bad the `TargetAttr` has no notion of `TargetInfo`; it's kind of ugly to have to pass this in.
Typically we don't have attributes be context/targetinfo aware.  It is a little sucky to have to pass this in, but I didn't think it too evil.


================
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;
+
----------------
nickdesaulniers wrote:
> 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.
Thats perhaps true.  I was going for consistency with "split", since that is what I copied it from :) 


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

https://reviews.llvm.org/D68050