hokein added inline comments.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:115
// Parameters for the GLR parsing.
+// FIXME: refine it with the ParseLang struct.
struct ParseParams {
----------------
sammccall wrote:
> You're already touching the callsites of the glr* functions, it might be
> worth just doing this already... up to you
these callsites are trivial to update. I'd prefer to do it in a separate patch.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:1
+//===--- ParseLang.h ------------------------------------------- -*-
C++-*-===//
+//
----------------
sammccall wrote:
> The "ParseLang" name doesn't feel right to me :-(
>
> I think it's a combination of:
> - Lang is unneccesarily abbreviated
> - "Parse" doesn't actually disambiguate much, as "parse" is the whole project
>
> Do you think `clang::pseudo::Language` would work?
>
>
> (Sorry for not realizing this on the previous pass, I know it's a pain...
> happy to chat more offline)
That sounds good to me. Regarding the location of this file, I think the subdir
grammar will be a better fit.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:19
+// Specify a language that can be parsed by the pseduoparser.
+// Manifest generated from a bnf grammar file.
+struct ParseLang {
----------------
sammccall wrote:
> I don't know what this sentence means - is it always true? is it necessary?
I think it is always true for the grammar and LRTable object, but might not be
true for the LangOptions. Removed.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h:24
+
+ // FIXME: add clang::LangOptions.
+ // FIXME: add default start symbols.
----------------
sammccall wrote:
> is this "fix right after this patch" or "fix sometime, maybe" or something in
> between?
>
> (I think these make a lot of sense, and am worried the structure will be
> incoherent if we don't actually add them soon)
I would expect we will add them soon.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:9
+//
+// A library shared among different pseudoparser-based tools. It provides a
+// uniform way to get basic pieces of the parser (Grammar, LRTable etc) from
----------------
sammccall wrote:
> The first sentence should say what it is, rather than what calls it.
>
> If the file by design defines a single thing function I'm not sure a file
> comment distinct from the function comment helps much, maybe merge them?
I'm not sure whether we will add more functions in this library (though I can't
come up with one), I will just keep the file comment.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h:27
+// Returns the corresponding language from the '--grammar' command-line flag.
+const ParseLang &getParseLangFromFlags();
+
----------------
sammccall wrote:
> this should generally be called exactly one from CLI tools - why does it need
> to be memoized and returned as a reference?
yeah, for now, returning an object is ok, but in the future, when we build the
ParseLang (LRTable, LRGrammar) for cxx.bnf in a fully-static way, we might have
trouble right?
================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:38
+ << "': " << EC.message() << "\n";
+ std::exit(1);
+ }
----------------
sammccall wrote:
> this is extremely surprising.
> At minimum it needs to be very clearly documented, but I think it's probably
> better to return null to the caller
It doesn't seem to be surprising to me, as this function is used for the CLI
tools, crashing the program when the input grammar text is invalid seems
reasonable (all our exiting CLI tools at the moment behave like this), and we
have print the error message.
Returning a nullptr seems annoying IMO -- it requires all callers to handle
this unimportant case. Updated the document for this function.
================
Comment at: clang-tools-extra/pseudo/lib/cli/CLI.cpp:42
+ auto G = Grammar::parseBNF(GrammarText->get()->getBuffer(), Diags);
+ if (!Diags.empty()) {
+ for (const auto &Diag : Diags)
----------------
sammccall wrote:
> this if() isn't needed unless you want to print a header to provide some
> context (which might be a good idea)
I don't get your point of the comment. Not sure what you meant by `Print a
header`?
I think for the CLI tool use-case, printing the diagnostic of the grammar by
default is reasonable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128679/new/
https://reviews.llvm.org/D128679
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
