This is an archive of the discontinued LLVM Phabricator instance.

[clang][AST] ASTNameGenerator: A refactoring of CodegenNameGeneratorImpl (NFC).
ClosedPublic

Authored by plotfi on Jun 18 2019, 8:02 PM.

Details

Summary

CodegenNameGeneratorImpl is the most complete implementation of name mangling but it is in clang::Index. This makes it very difficult to use it elsewhere in clang. This is just a NFC refactoring of CodegenNameGeneratorImpl into a class called ASTNameGenerator. Other than the name and placement change there is no functional change.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Jun 18 2019, 8:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 8:02 PM
compnerd added inline comments.Jun 19 2019, 8:57 AM
clang/include/clang/AST/Mangle.h
19 ↗(On Diff #205500)

Move these into the implementation, forward declarations should be sufficient.

21 ↗(On Diff #205500)

This isn't used, it belongs in the implementation.

25 ↗(On Diff #205500)

Is Mangler used in the header? I think this can be in the implementation.

253 ↗(On Diff #205500)

I think I prefer ASTNameGenerator or even ASTNameMangler (though I am partial to Microsoft's term: "decoration").

plotfi updated this revision to Diff 205625.Jun 19 2019, 9:59 AM

Addressing @compnerd's feedback.

plotfi retitled this revision from [clang][AST] MangleUtil: A refactoring of CodegenNameGeneratorImpl (NFC). to [clang][AST] ASTNameGenerator: A refactoring of CodegenNameGeneratorImpl (NFC)..Jun 19 2019, 9:59 AM
plotfi edited the summary of this revision. (Show Details)
plotfi marked 4 inline comments as done.Jun 19 2019, 10:06 AM
aaron.ballman added inline comments.Jun 19 2019, 10:10 AM
clang/include/clang/AST/Mangle.h
249–250 ↗(On Diff #205625)

Do these still need to be public members? Back when this class was an implementation detail, that didn't matter as much, but I'd prefer a cleaner public interface before we expose this to other consumers.

compnerd added inline comments.Jun 19 2019, 10:17 AM
clang/include/clang/AST/Mangle.h
248 ↗(On Diff #205625)

Please make this a class and hide the members, exposing just the constructor and the 3 methods as @aaron.ballman pointed out that this is a struct not a class! I missed that.

plotfi marked an inline comment as done.Jun 19 2019, 10:20 AM
plotfi added inline comments.
clang/include/clang/AST/Mangle.h
249–250 ↗(On Diff #205625)

Good catch! This was a struct when it was CodegenNameGeneratorImpl but these were hidden by the mere fact that they were inside the .cpp file and not in a header. Thanks!

plotfi updated this revision to Diff 205643.Jun 19 2019, 11:08 AM

Update to address @aaron.ballman's catch that ASTNameGenerator was not private enough.

plotfi marked 2 inline comments as done.Jun 19 2019, 11:09 AM
compnerd accepted this revision.Jun 19 2019, 11:17 AM
This revision is now accepted and ready to land.Jun 19 2019, 11:17 AM
aaron.ballman accepted this revision.Jun 19 2019, 11:37 AM

LGTM with a few nits.

clang/include/clang/AST/Mangle.h
253 ↗(On Diff #205643)

Slight preference to make this constructor explicit since it only accepts a single argument and I can't imagine wanting a converting constructor there.

clang/lib/AST/Mangle.cpp
25 ↗(On Diff #205643)

Do we have to link in any new libraries in CMake for this new dependency?

plotfi marked 3 inline comments as done.Jun 19 2019, 11:41 AM
plotfi added inline comments.
clang/lib/AST/Mangle.cpp
25 ↗(On Diff #205643)

I don't believe so. It builds and make check-clangs. I will try a shared lib build too. It looks like AST things are already included, and LLVM things are already included so I doubt any new library dep change is needed at all.

plotfi marked an inline comment as done.Jun 19 2019, 12:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 1:49 PM