Page MenuHomePhabricator

[AST] De-duplicate empty node introspection
ClosedPublic

Authored by steveire on Mar 17 2021, 4:40 AM.

Details

Summary

This way we can add support for other nodes without duplication.

Diff Detail

Event Timeline

steveire created this revision.Mar 17 2021, 4:40 AM
steveire requested review of this revision.Mar 17 2021, 4:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 17 2021, 4:40 AM

Why were there two copies in the first place? What was the copy in the cmake file good for, why wasn't the one written by generate_cxx_src_locs.py sufficient? But pulling it into its own file seems nicer than having it inline in python or cmake code.

Another observation: it's a bit strange that the empty impl codepath doesn't share any code with the non-empty code path. The py script could do something like

if use_empty_implementation:
  jsonData = { classesInClade': [] 'classNames': [] }

and then just fall through to the normal generation code and everything should in theory work with much fewer special cases, right?

(What's a "clade"?)

(I still think the whole approach feels off, as mentioned on the other review.)

clang/lib/Tooling/CMakeLists.txt
38

Is configure_file(COYPONLY) different from file(COPY)? In what way?

Are you expecting to use cmake vars in the .in file eventually?

89

You need to add a dep on the inc file here so that this step reruns if EmptyNodeIntrospection.inc changes.

96

What's the advantage of making a copy above? Why not call the checked-in file EmptyNodeIntrospection.inc and pass the path to it directly here?

clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py
176

Shouldn't this be f.write(open(options.empty_implementation).read())?

If you're changing this, you could also make it so that it only writes the output if it'd be different from what it's there, with something like this:

llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn
8

Don't worry about the gn build, just don't update it at all.

(The input file would have to go into inputs for correct incremental builds, the rebase_path looks not quite right, there's nothing that copies the .inc.in file to .inc, etc)

steveire marked 4 inline comments as done.Mar 17 2021, 10:15 AM

Why were there two copies in the first place? What was the copy in the cmake file good for, why wasn't the one written by generate_cxx_src_locs.py sufficient?

AFAIK python is not a hard-requirement of the llvm/clang build.

But pulling it into its own file seems nicer than having it inline in python or cmake code.

Yes.

Another observation: it's a bit strange that the empty impl codepath doesn't share any code with the non-empty code path. The py script could do something like

if use_empty_implementation:
  jsonData = { classesInClade': [] 'classNames': [] }

and then just fall through to the normal generation code and everything should in theory work with much fewer special cases, right?

I don't think this cuts down on special cases. Also, it looks like it would generate code with usings which are never used

using LocationAndString = SourceLocationMap::value_type;
using RangeAndString = SourceRangeMap::value_type;

which would lead us to having to handle "unused using" warnings.

Copying the file seems a good compromise between the options.

(What's a "clade"?)

The group of types which have a common ancestor. Everything inheriting from Stmt is in a different clade to everything inheriting from Decl.

clang/lib/Tooling/CMakeLists.txt
38

configure_file only copies the file if it is different. file(COPY) does too, but it doesn't seem to include a way to specify the destination file name.

96

I'm not sure what you mean. The configure_file is inside the if and this line is inside the else. Does that clear it up?

steveire marked an inline comment as done.Mar 20 2021, 9:30 AM

@thakis Do you have any more on this? Can we de-duplicate?

thakis accepted this revision.Apr 20 2021, 8:44 AM

Definitely a step forward, so lg :)

AFAIK python is not a hard-requirement of the llvm/clang build.

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

clang/lib/Tooling/CMakeLists.txt
96

Oh, I see. Maybe we could always call the py script and only make it copy through the empty file if the script isn't supposed to do anything?

This revision is now accepted and ready to land.Apr 20 2021, 8:44 AM
This revision was automatically updated to reflect the committed changes.

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

Thanks, I'll look into that soon and see how the buildbots respond.

I think it is? https://llvm.org/docs/GettingStarted.html#software lists it at least.

Thanks, I'll look into that soon and see how the buildbots respond.

I looked into a bit too, and at least gen_decision_forest in clang-tools-extra uses python without a guard. See clang-tools-extra/clangd/quality/CompletionModel.cmake and clang-tools-extra/clangd/CMakeLists.txt