[PATCH] D126536: [pseudo] Add grammar annotations support.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I addressed comments on the grammar part, (the remaining GLR, pseudo_gen parts are not covered yet), I think it would be better them into different patches, so that we can land the grammar bit first, then start doing the error recovery and guard implementation in parallel.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h:110
+  Rule(SymbolID Target, llvm::ArrayRef<SymbolID> Seq,
+       AttributeID Guard = NoneAttribute);
 
----------------
sammccall wrote:
> if this optional and rarely set, I think it's should be a constructor parameter - this constructor could become unwieldy.
> 
> It also forces you to process the attributes before creating the Rule, and reading the code doing it afterwards seems more natural.
I suppose you mean we should drop the Guard parameter in the constructor, yeah, that sounds good to me, and simplifies the BNF parsing bit.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:52
+    llvm::DenseSet<llvm::StringRef> UniqueAttrKeys;
+    llvm::DenseSet<llvm::StringRef> UniqueAttrValues = {""};
+    llvm::DenseMap<llvm::StringRef, AttributeID> AttrValueIDs;
----------------
sammccall wrote:
> why ""?
This was for the sentinel default 0 attribute value.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:62
+    auto ConsiderAttrValue = [&](llvm::StringRef AttrValueName) {
+      if (!AttrValueIDs.count(AttrValueName))
+        UniqueAttrValues.insert(AttrValueName);
----------------
sammccall wrote:
> AttrValueIDs never seems to get populated, so I'm not sure this works.
oops, this part of code wasn't fully cleaned up. AttrValueIDs is not needed indeed. 


================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:119
+
+        for (const auto& AttrKeyAndValue : Elt.Attributes) {
+          switch(AttrKeyAndValue.first) {
----------------
sammccall wrote:
> sammccall wrote:
> > This doesn't seem worth diagnosing to me - it seems stylistically weird but not really a problem to put a guard wherever.
> maybe another function to pull out here?
> applyAttribute(StringRef Name, StringRef Value, AttributeID ValueID, Rule&, Element&)
sounds good, move the extension bits to a dedicate `applyExtension`.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:119
+
+        for (const auto& AttrKeyAndValue : Elt.Attributes) {
+          switch(AttrKeyAndValue.first) {
----------------
hokein wrote:
> sammccall wrote:
> > sammccall wrote:
> > > This doesn't seem worth diagnosing to me - it seems stylistically weird but not really a problem to put a guard wherever.
> > maybe another function to pull out here?
> > applyAttribute(StringRef Name, StringRef Value, AttributeID ValueID, Rule&, Element&)
> sounds good, move the extension bits to a dedicate `applyExtension`.
> This doesn't seem worth diagnosing to me - it seems stylistically weird but not really a problem to put a guard wherever.

It is valid in syntax level, but not in semantic level, I think this is confusing -- the bnf parser doesn't emit any warning on this usage, just silently ignore them (grammar writer might think the grammar is correct, guard should work even putting it in the middle of the rules).

On the other hand, as you mentioned, grammar file is not user-faced, and we're the current authors, it seems ok to not do it (in favour of simplicity) right now (we might need to reconsider improving it there are new grammar contributors).




================
Comment at: clang-tools-extra/pseudo/lib/grammar/GrammarBNF.cpp:254
         continue; // skip empty
+      if (Chunk.startswith("[") && Chunk.endswith("]")) {
+        if (Out.Sequence.empty()) {
----------------
sammccall wrote:
> Again, I don't think we should be aiming to provide nice diagnostics for each possible way we could get this wrong - the grammar here is part of the pseudo-parser, not user input.
fair enough, but we should keep minimal critical diagnostics at least to avoid shooting ourself in the foot. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126536