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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
