aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/Attr.td:268 def TargetMips : TargetArch<["mips", "mipsel"]>; +def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>; def TargetMSP430 : TargetArch<["msp430"]>; ---------------- Can you also rename `TargetMips` to something that distinguishes it from `TargetAnyMips`, like we have with `TargetX86` vs `TargetAnyX86`? I'd be hard-pressed to notice the difference between these two target names when reviewing an attribute, but I'm not familiar enough with the MIPS architecture to have a particularly good suggestion for improvement. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:5979-5981 + if (S.Context.getTargetInfo().getABI() != "o32") + S.Diag(Attr.getLoc(), diag::err_attribute_supported_by_o32) + << Attr.getName() << D->getLocation(); ---------------- atanasyan wrote: > aaron.ballman wrote: > > Please do not put logic into the switch statement, but instead sink it into > > a helper function. We hope to remove the boilerplate switch someday, so > > keeping with the `handleFooAttr()` pattern is preferred. Same below. > > > > Further, why is this not handled by a TargetSpecificAttr in Attr.td? See > > `TargetMicrosoftCXXABI` and `MSNoVTable` for an example. > > Further, why is this not handled by a TargetSpecificAttr in Attr.td? See > > TargetMicrosoftCXXABI and MSNoVTable for an example. > > I thought about this, but in that case compiler will show "unknown attribute > 'mips16' ignored" warning in case of using `mips16` on MIPS64 targets. As to > me this warning is not quite correct in that case because `mips16`, > `micromips`, `interrupt` all are known MIPS-specific attributes, but not > applicable on MIPS64 by various reasons. > As to me this warning is not quite correct in that case because mips16, > micromips, interrupt all are known MIPS-specific attributes, but not > applicable on MIPS64 by various reasons. The same is true of all target-specific attributes -- they're known to the compiler, but their usage is not known on that specific target architecture. I would not be opposed to a patch that distinguished between the two situations, however (completely unknown attribute/and attribute that's known to be prohibited). Repository: rL LLVM https://reviews.llvm.org/D36208 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
