This is an archive of the discontinued LLVM Phabricator instance.

[pseudo] Define a clangPseudoCLI library.
ClosedPublic

Authored by hokein on Jun 27 2022, 1:21 PM.

Details

Summary
  • define a common data structure ParseLang which is a compiled result of the bnf grammar. It is defined in ParseLang.h;
  • creates a clangPseudoCLI lib which defines a grammar commandline flag and expose a function to get the ParseLang. It supports --grammar=cxx, --grammmar=/path/to/file.bnf;
  • Use the clangPseudoCLI in clang-pseudo, fuzzer, and benchmark tools (simplify the code and use the prebuilt cxx grammar);

Split out from https://reviews.llvm.org/D127448 with comments addressed

Diff Detail

Event Timeline

hokein created this revision.Jun 27 2022, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 1:21 PM
Herald added a subscriber: mgorny. · View Herald Transcript
hokein requested review of this revision.Jun 27 2022, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 1:21 PM
hokein updated this revision to Diff 440366.Jun 27 2022, 1:32 PM

remove a dependency from pseudoCLI lib

Thanks, the split here looks good!

clang-tools-extra/pseudo/include/clang-pseudo/GLR.h
115

You're already touching the callsites of the glr* functions, it might be worth just doing this already... up to you

clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h
1 ↗(On Diff #440366)

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)

19 ↗(On Diff #440366)

I don't know what this sentence means - is it always true? is it necessary?

24 ↗(On Diff #440366)

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)

clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h
10

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?

28

this should generally be called exactly one from CLI tools - why does it need to be memoized and returned as a reference?

clang-tools-extra/pseudo/lib/cli/CLI.cpp
39

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

43

this if() isn't needed unless you want to print a header to provide some context (which might be a good idea)

hokein updated this revision to Diff 440765.Jun 28 2022, 2:06 PM
hokein marked an inline comment as done.

address comments

hokein added inline comments.Jun 28 2022, 2:09 PM
clang-tools-extra/pseudo/include/clang-pseudo/GLR.h
115

these callsites are trivial to update. I'd prefer to do it in a separate patch.

clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h
1 ↗(On Diff #440366)

That sounds good to me. Regarding the location of this file, I think the subdir grammar will be a better fit.

19 ↗(On Diff #440366)

I think it is always true for the grammar and LRTable object, but might not be true for the LangOptions. Removed.

24 ↗(On Diff #440366)

I would expect we will add them soon.

clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h
10

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.

28

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?

clang-tools-extra/pseudo/lib/cli/CLI.cpp
39

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.

43

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.

sammccall accepted this revision.Jun 29 2022, 1:47 AM
sammccall added inline comments.
clang-tools-extra/pseudo/benchmarks/Benchmark.cpp
51

nit: still PLang here and in a bunch of places

clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h
1 ↗(On Diff #440366)

The main purpose of the grammar library is to minimize the amount of stuff we pull into the gen step right?

I'm a bit concerned about mixing clang::LangOptions in there unneccesarily - if grammar doesn't *need* the header, maybe it's OK where it is?

clang-tools-extra/pseudo/include/clang-pseudo/cli/CLI.h
9

nit: "this file implements a CLI library which provides an interface for" is boilerplate.
Just "Provides the Grammar, LRTable etc for a language specified by flags"?

clang-tools-extra/pseudo/lib/cli/CLI.cpp
43

You could replace

if (!Diags.empty())
 for(D : Diags)
    ...

with just:

for (D : Diags)
  ...

unless you would prefer:

if (!Diags.empty()) {
  errs() << "Problems with the grammar:\n";
  for (D : Diags)
    ...
}

(Yesterday I thought the last one would be clearer, today I'm not so sure)

This revision is now accepted and ready to land.Jun 29 2022, 1:47 AM
hokein updated this revision to Diff 440929.Jun 29 2022, 4:03 AM
hokein marked an inline comment as done.

address review comments

clang-tools-extra/pseudo/benchmarks/Benchmark.cpp
51

renamed all to Lang

clang-tools-extra/pseudo/include/clang-pseudo/ParseLang.h
1 ↗(On Diff #440366)

as discussed offline, moved back to pseudo/

clang-tools-extra/pseudo/lib/cli/CLI.cpp
43

oops, I see what you mean now.

sammccall accepted this revision.Jun 29 2022, 4:46 AM

Looks good!

clang-tools-extra/pseudo/include/clang-pseudo/Language.h
21 ↗(On Diff #440929)

LR might be more distinguishing (and shorter!), up to you

This revision was landed with ongoing or failed builds.Jun 30 2022, 11:31 PM
This revision was automatically updated to reflect the committed changes.