Re: [PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
On Thu, Aug 30, 2018 at 4:12 PM, Richard Smith wrote:
> On Thu, 30 Aug 2018 at 12:27, Aaron Ballman via cfe-commits
> wrote:
>>
>> On Thu, Aug 30, 2018 at 3:21 PM, Richard Smith - zygoloid via
>> Phabricator wrote:
>> > rsmith marked an inline comment as done.
>> > rsmith added inline comments.
>> >
>> >
>> >
>> > Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881
>> > +SpellingKind K = (SpellingKind)Kind;
>> > +// FIXME: Why are Microsoft spellings not listed?
>> > +if (K == SpellingKind::Microsoft)
>> >
>> > aaron.ballman wrote:
>> >> We don't actually support Microsoft's attribute spellings currently and
>> >> have no attributes there to document. I think the fixme should probably
>> >> read
>> >> "TODO: support documenting Microsoft spellings" or something more
>> >> concrete.
>> > Done. (I accidentally pushed the old version, so this is done in
>> > r341100.)
>> >
>> > For what it's worth, we have one `Microsoft` spelling listed in the .td
>> > file already (but I assume this has no effect):
>> >
>> > ```
>> > def Uuid : InheritableAttr {
>> > let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
>> > ```
>>
>> Hmm, I take it back, we do support a Microsoft attribute, only to warn
>> about it being deprecated and telling users to use __declspec instead:
>> https://godbolt.org/z/_0ZxWq
>>
>> I remember when we tried to add more support for parsing Microsoft
>> attributes, but I had the impression we didn't support them beyond the
>> very basics of parsing. Perhaps we do want to document them though,
>> since there's at least one?
>
>
> Given that doing so will make the "supported syntaxes" table wider for all
> attributes, and it's already about as wide as seems reasonable, and we
> consider all attributes of this form to be deprecated, I don't think it's
> worth it. Maybe if we only included non-empty columns in the syntax table?
I think for right now I'd prefer to keep a consistent syntax table
layout. It makes it easier to visually scan for information when all
the columns roughly line up vertically, and we only support this one
spelling just to tell users not to use it anyway. We can try out the
column approach later if we find need.
~Aaron
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
On Thu, 30 Aug 2018 at 12:27, Aaron Ballman via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> On Thu, Aug 30, 2018 at 3:21 PM, Richard Smith - zygoloid via
> Phabricator wrote:
> > rsmith marked an inline comment as done.
> > rsmith added inline comments.
> >
> >
> >
> > Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881
> > +SpellingKind K = (SpellingKind)Kind;
> > +// FIXME: Why are Microsoft spellings not listed?
> > +if (K == SpellingKind::Microsoft)
> >
> > aaron.ballman wrote:
> >> We don't actually support Microsoft's attribute spellings currently and
> have no attributes there to document. I think the fixme should probably
> read "TODO: support documenting Microsoft spellings" or something more
> concrete.
> > Done. (I accidentally pushed the old version, so this is done in
> r341100.)
> >
> > For what it's worth, we have one `Microsoft` spelling listed in the .td
> file already (but I assume this has no effect):
> >
> > ```
> > def Uuid : InheritableAttr {
> > let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
> > ```
>
> Hmm, I take it back, we do support a Microsoft attribute, only to warn
> about it being deprecated and telling users to use __declspec instead:
> https://godbolt.org/z/_0ZxWq
>
> I remember when we tried to add more support for parsing Microsoft
> attributes, but I had the impression we didn't support them beyond the
> very basics of parsing. Perhaps we do want to document them though,
> since there's at least one?
>
Given that doing so will make the "supported syntaxes" table wider for all
attributes, and it's already about as wide as seems reasonable, and we
consider all attributes of this form to be deprecated, I don't think it's
worth it. Maybe if we only included non-empty columns in the syntax table?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
On Thu, Aug 30, 2018 at 3:21 PM, Richard Smith - zygoloid via
Phabricator wrote:
> rsmith marked an inline comment as done.
> rsmith added inline comments.
>
>
>
> Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881
> +SpellingKind K = (SpellingKind)Kind;
> +// FIXME: Why are Microsoft spellings not listed?
> +if (K == SpellingKind::Microsoft)
>
> aaron.ballman wrote:
>> We don't actually support Microsoft's attribute spellings currently and have
>> no attributes there to document. I think the fixme should probably read
>> "TODO: support documenting Microsoft spellings" or something more concrete.
> Done. (I accidentally pushed the old version, so this is done in r341100.)
>
> For what it's worth, we have one `Microsoft` spelling listed in the .td file
> already (but I assume this has no effect):
>
> ```
> def Uuid : InheritableAttr {
> let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
> ```
Hmm, I take it back, we do support a Microsoft attribute, only to warn
about it being deprecated and telling users to use __declspec instead:
https://godbolt.org/z/_0ZxWq
I remember when we tried to add more support for parsing Microsoft
attributes, but I had the impression we didn't support them beyond the
very basics of parsing. Perhaps we do want to document them though,
since there's at least one?
~Aaron
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D51473
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
rsmith marked an inline comment as done.
rsmith added inline comments.
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881
+SpellingKind K = (SpellingKind)Kind;
+// FIXME: Why are Microsoft spellings not listed?
+if (K == SpellingKind::Microsoft)
aaron.ballman wrote:
> We don't actually support Microsoft's attribute spellings currently and have
> no attributes there to document. I think the fixme should probably read
> "TODO: support documenting Microsoft spellings" or something more concrete.
Done. (I accidentally pushed the old version, so this is done in r341100.)
For what it's worth, we have one `Microsoft` spelling listed in the .td file
already (but I assume this has no effect):
```
def Uuid : InheritableAttr {
let Spellings = [Declspec<"uuid">, Microsoft<"uuid">];
```
Repository:
rL LLVM
https://reviews.llvm.org/D51473
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341097: Improve attribute documentation to list which
spellings are used in which… (authored by rsmith, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D51473
Files:
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
Index: cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
===
--- cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
+++ cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
@@ -3747,18 +3747,66 @@
getPragmaAttributeSupport(Records).generateParsingHelpers(OS);
}
+enum class SpellingKind {
+ GNU,
+ CXX11,
+ C2x,
+ Declspec,
+ Microsoft,
+ Keyword,
+ Pragma,
+};
+static const size_t NumSpellingKinds = (size_t)SpellingKind::Pragma + 1;
+
+class SpellingList {
+ std::vector Spellings[NumSpellingKinds];
+
+public:
+ ArrayRef operator[](SpellingKind K) const {
+return Spellings[(size_t)K];
+ }
+
+ void add(const Record , FlattenedSpelling Spelling) {
+SpellingKind Kind = StringSwitch(Spelling.variety())
+.Case("GNU", SpellingKind::GNU)
+.Case("CXX11", SpellingKind::CXX11)
+.Case("C2x", SpellingKind::C2x)
+.Case("Declspec", SpellingKind::Declspec)
+.Case("Microsoft", SpellingKind::Microsoft)
+.Case("Keyword", SpellingKind::Keyword)
+.Case("Pragma", SpellingKind::Pragma);
+std::string Name;
+if (!Spelling.nameSpace().empty()) {
+ switch (Kind) {
+ case SpellingKind::CXX11:
+ case SpellingKind::C2x:
+Name = Spelling.nameSpace() + "::";
+break;
+ case SpellingKind::Pragma:
+Name = Spelling.nameSpace() + " ";
+break;
+ default:
+PrintFatalError(Attr.getLoc(), "Unexpected namespace in spelling");
+ }
+}
+Name += Spelling.name();
+
+Spellings[(size_t)Kind].push_back(Name);
+ }
+};
+
class DocumentationData {
public:
const Record *Documentation;
const Record *Attribute;
std::string Heading;
- unsigned SupportedSpellings;
+ SpellingList SupportedSpellings;
DocumentationData(const Record , const Record ,
-const std::pair HeadingAndKinds)
+std::pair HeadingAndSpellings)
: Documentation(), Attribute(),
-Heading(std::move(HeadingAndKinds.first)),
-SupportedSpellings(HeadingAndKinds.second) {}
+Heading(std::move(HeadingAndSpellings.first)),
+SupportedSpellings(std::move(HeadingAndSpellings.second)) {}
};
static void WriteCategoryHeader(const Record *DocCategory,
@@ -3774,28 +3822,21 @@
OS << "\n\n";
}
-enum SpellingKind {
- GNU = 1 << 0,
- CXX11 = 1 << 1,
- C2x = 1 << 2,
- Declspec = 1 << 3,
- Microsoft = 1 << 4,
- Keyword = 1 << 5,
- Pragma = 1 << 6
-};
-
-static std::pair
-GetAttributeHeadingAndSpellingKinds(const Record ,
-const Record ) {
+static std::pair
+GetAttributeHeadingAndSpellings(const Record ,
+const Record ) {
// FIXME: there is no way to have a per-spelling category for the attribute
// documentation. This may not be a limiting factor since the spellings
// should generally be consistently applied across the category.
std::vector Spellings = GetFlattenedSpellings(Attribute);
+ if (Spellings.empty())
+PrintFatalError(Attribute.getLoc(),
+"Attribute has no supported spellings; cannot be "
+"documented");
// Determine the heading to be used for this attribute.
std::string Heading = Documentation.getValueAsString("Heading");
- bool CustomHeading = !Heading.empty();
if (Heading.empty()) {
// If there's only one spelling, we can simply use that.
if (Spellings.size() == 1)
@@ -3819,76 +3860,42 @@
PrintFatalError(Attribute.getLoc(),
"This attribute requires a heading to be specified");
- // Gather a list of unique spellings; this is not the same as the semantic
- // spelling for the attribute. Variations in underscores and other non-
- // semantic characters are still acceptable.
- std::vector Names;
-
- unsigned SupportedSpellings = 0;
- for (const auto : Spellings) {
-SpellingKind Kind = StringSwitch(I.variety())
-.Case("GNU", GNU)
-.Case("CXX11", CXX11)
-.Case("C2x", C2x)
-.Case("Declspec", Declspec)
-.Case("Microsoft", Microsoft)
-.Case("Keyword", Keyword)
-.Case("Pragma", Pragma);
-
-// Mask in the supported spelling.
-SupportedSpellings |= Kind;
+
[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a commenting nit. Thank you for this, I like this exposition better! Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881 +SpellingKind K = (SpellingKind)Kind; +// FIXME: Why are Microsoft spellings not listed? +if (K == SpellingKind::Microsoft) We don't actually support Microsoft's attribute spellings currently and have no attributes there to document. I think the fixme should probably read "TODO: support documenting Microsoft spellings" or something more concrete. Repository: rC Clang https://reviews.llvm.org/D51473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51473: Improve attribute documentation to list which spellings are used in which syntaxes.
rsmith created this revision.
rsmith added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.
Instead of listing all the spellings (including attribute namespaces) in
the section heading, only list the actual attribute names there, and
list the spellings in the supported syntaxes table.
This allows us to properly describe things like [[fallthrough]], for
which we allow a clang:: prefix in C++ but not in C, and AlwaysInline,
which has one spelling as a GNU attribute and a different spelling as a
keyword, without needing to repeat the syntax description in the
documentation text.
Sample rendering: https://pste.eu/p/T1ZV.html
Repository:
rC Clang
https://reviews.llvm.org/D51473
Files:
include/clang/Basic/AttrDocs.td
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -3747,18 +3747,66 @@
getPragmaAttributeSupport(Records).generateParsingHelpers(OS);
}
+enum class SpellingKind {
+ GNU,
+ CXX11,
+ C2x,
+ Declspec,
+ Microsoft,
+ Keyword,
+ Pragma,
+};
+static const size_t NumSpellingKinds = (size_t)SpellingKind::Pragma + 1;
+
+class SpellingList {
+ std::vector Spellings[NumSpellingKinds];
+
+public:
+ ArrayRef operator[](SpellingKind K) const {
+return Spellings[(size_t)K];
+ }
+
+ void add(const Record , FlattenedSpelling Spelling) {
+SpellingKind Kind = StringSwitch(Spelling.variety())
+.Case("GNU", SpellingKind::GNU)
+.Case("CXX11", SpellingKind::CXX11)
+.Case("C2x", SpellingKind::C2x)
+.Case("Declspec", SpellingKind::Declspec)
+.Case("Microsoft", SpellingKind::Microsoft)
+.Case("Keyword", SpellingKind::Keyword)
+.Case("Pragma", SpellingKind::Pragma);
+std::string Name;
+if (!Spelling.nameSpace().empty()) {
+ switch (Kind) {
+ case SpellingKind::CXX11:
+ case SpellingKind::C2x:
+Name = Spelling.nameSpace() + "::";
+break;
+ case SpellingKind::Pragma:
+Name = Spelling.nameSpace() + " ";
+break;
+ default:
+PrintFatalError(Attr.getLoc(), "Unexpected namespace in spelling");
+ }
+}
+Name += Spelling.name();
+
+Spellings[(size_t)Kind].push_back(Name);
+ }
+};
+
class DocumentationData {
public:
const Record *Documentation;
const Record *Attribute;
std::string Heading;
- unsigned SupportedSpellings;
+ SpellingList SupportedSpellings;
DocumentationData(const Record , const Record ,
-const std::pair HeadingAndKinds)
+std::pair HeadingAndSpellings)
: Documentation(), Attribute(),
-Heading(std::move(HeadingAndKinds.first)),
-SupportedSpellings(HeadingAndKinds.second) {}
+Heading(std::move(HeadingAndSpellings.first)),
+SupportedSpellings(std::move(HeadingAndSpellings.second)) {}
};
static void WriteCategoryHeader(const Record *DocCategory,
@@ -3774,28 +3822,21 @@
OS << "\n\n";
}
-enum SpellingKind {
- GNU = 1 << 0,
- CXX11 = 1 << 1,
- C2x = 1 << 2,
- Declspec = 1 << 3,
- Microsoft = 1 << 4,
- Keyword = 1 << 5,
- Pragma = 1 << 6
-};
-
-static std::pair
-GetAttributeHeadingAndSpellingKinds(const Record ,
-const Record ) {
+static std::pair
+GetAttributeHeadingAndSpellings(const Record ,
+const Record ) {
// FIXME: there is no way to have a per-spelling category for the attribute
// documentation. This may not be a limiting factor since the spellings
// should generally be consistently applied across the category.
std::vector Spellings = GetFlattenedSpellings(Attribute);
+ if (Spellings.empty())
+PrintFatalError(Attribute.getLoc(),
+"Attribute has no supported spellings; cannot be "
+"documented");
// Determine the heading to be used for this attribute.
std::string Heading = Documentation.getValueAsString("Heading");
- bool CustomHeading = !Heading.empty();
if (Heading.empty()) {
// If there's only one spelling, we can simply use that.
if (Spellings.size() == 1)
@@ -3819,76 +3860,42 @@
PrintFatalError(Attribute.getLoc(),
"This attribute requires a heading to be specified");
- // Gather a list of unique spellings; this is not the same as the semantic
- // spelling for the attribute. Variations in underscores and other non-
- // semantic characters are still acceptable.
- std::vector Names;
+ SpellingList SupportedSpellings;
+ for (const auto : Spellings)
+SupportedSpellings.add(Attribute, I);
- unsigned SupportedSpellings = 0;
- for (const auto : Spellings) {
-SpellingKind Kind = StringSwitch(I.variety())
