[PATCH] D46805: If some platforms do not support an attribute, we should exclude the platform

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 5 01:48:38 PDT 2018


aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a nit with one of the tests. Once you've updated the patch and verified that check-clang passes all tests, I can commit for you next week when I'm back from meetings (unless someone else gets to it before me).



================
Comment at: include/clang/Basic/Attr.td:322
+def TargetSupportsAlias : TargetSpec {
+  let ObjectFormats = ["COFF", "ELF", "Wasm"];
+}
----------------
HLJ2009 wrote:
> aaron.ballman wrote:
> > Did you verify that Wasm supports the alias attribute? If it is supported, it might be nice to add a test to `CodeGen/alias.c` to demonstrate it. Similar for COFF.
> Yes, I used the following command line to test my test file.
> clang -cc1 -triple wasm32-unknown-unknown -fsyntax-only -verify attr-alias-has.c
> clang -cc1 -triple wasm64-unknown-unknown -fsyntax-only -verify attr-alias-has.c
> The test result is ok. 
> ok, I will update it.
That's perhaps not the "support" I was talking about. I didn't mean "are you sure the target spec on the attribute is doing what you expect", but "are you sure that alias works and has the expected semantics when the target is Wasm"?

However, @sunfish already said it's intended to be supported in Wasm, so I think that's fine.


================
Comment at: include/clang/Basic/Attr.td:564
   let Subjects = SubjectList<[Function, GlobalVar], ErrorDiag>;
   let Documentation = [Undocumented];
 }
----------------
Now that we're touching this attribute so that it no longer is available on some targets, it would be good to consider adding documentation for the attribute. That documentation isn't your responsibility and doesn't need to be part of this patch, but if it was something you wanted to tackle in a follow-up, that'd be appreciated (but not required).


================
Comment at: test/Sema/attr-alias-has.c:13
+#endif
\ No newline at end of file

----------------
Please add a newline to the end of the file.


Repository:
  rC Clang

https://reviews.llvm.org/D46805





More information about the cfe-commits mailing list