Page MenuHomePhabricator

[clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id
ClosedPublic

Authored by nridge on Jun 21 2021, 12:19 AM.

Diff Detail

Event Timeline

nridge requested review of this revision.Jun 21 2021, 12:19 AM
nridge created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 12:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This'll need a test case & does the change pass all existing tests?
Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (and you can shorten the bug reference to "PR50774" rather than the whole bugs.llvm.org URL)

nridge added a comment.EditedJun 21 2021, 5:55 PM

Thanks for having a look!

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

& does the change pass all existing tests?

I mainly pushed the patch to Phabricator in this WIP form because I was hoping that would trigger some sort of CI run that would tell me that :) (I tried running ninja check-clang locally, but that was looking to take a very long time (like 2+ hours) to run locally.)

It's not clear to me if that actually happened? I see something about "pre-merge checks" in the revision metadata (and they're green), but it's not clear to me what test suites it actually ran.

nridge retitled this revision from [clang] [WIP] Fix for https://bugs.llvm.org/show_bug.cgi?id=50774 to [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id.Jun 21 2021, 5:56 PM
nridge edited the summary of this revision. (Show Details)

Thanks for having a look!

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

One way you could find some places this might be tested would be to introduce a deliberate break in the code somehow - like changing this parameter to be "true" instead of false - and then running the tests to see what fails. But I think the problem is likely that the tests all want the existing behavior/never tweak the Policy in this context.

So likely what you'd need would be a unit test of some kind (check around in clang/unittests to see if there are other AST pretty printer tests, for instance).

& does the change pass all existing tests?

I mainly pushed the patch to Phabricator in this WIP form because I was hoping that would trigger some sort of CI run that would tell me that :) (I tried running ninja check-clang locally, but that was looking to take a very long time (like 2+ hours) to run locally.)

Hmm - what sort of machine config do you have? What build configuration were you building/running with?

It's not clear to me if that actually happened? I see something about "pre-merge checks" in the revision metadata (and they're green), but it's not clear to me what test suites it actually ran.

Yeah, it looks like some tests ran: https://buildkite.com/llvm-project/premerge-checks/builds/44109 (if you follow from the "Build 157317: pre-merge checks" under the patch description, then copy/paste the URL from somewhere in there you'll get to that build info

nridge updated this revision to Diff 355103.Jun 28 2021, 8:33 PM

add test

nridge retitled this revision from [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id to [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id.Jun 28 2021, 8:34 PM
nridge added a reviewer: dblaikie.

So likely what you'd need would be a unit test of some kind (check around in clang/unittests to see if there are other AST pretty printer tests, for instance).

Thanks for the suggestion! I did find existing pretty printer tests and was able to write a test for this fix by doing something similar.

I added you as reviewer for the patch, hope that's ok.

nridge updated this revision to Diff 355104.Jun 28 2021, 8:37 PM

fix comment at top of file

The test code seems a bit overly general/overengineered for the use case, perhaps? (for instance it has unused parameters like the "Args" parameter - that neither caller passes in a value for, it has accessors (GetPRinted/getNumFoundTypes) when all the code fits on a (largeish) screen & so maybe having public members might be fine for that purpose - maybe all those are boilerplate AST matcher stuff, I guess, which is OK? but wouldn't mind it a bit simpler given the simplicity of the test

& perhaps this could be tested without the full power of AST matchers? Though perhaps that is the easiest way to locate the desired AST node.

clang/unittests/AST/TypePrinterTest.cpp
27–28

this is probably unnecessary/redundant - an llvm::function_ref itself can already be empty/none/boolean tested, which should be adequate without wrapping it in the extra layer of Optional?

But also, since there's only two callers and they both pass a non-empty functor - seems fine not to even test for empty and always apply the functor.

Alternatively, the caller could pass in a PrintingPolicy, instead of mutating the one here? Might be simpler?

I based the infrastructure closely on that in DeclPrinterTest.cpp and StmtPrinterTest.cpp. I figured the general framework could come in useful to people adding new type printing tests in the future.

I'm happy to trim it down a bit (e.g. remove unused methods and such), but in terms of the general approach, I feel like there's value in being consistent with the other printer tests?

I based the infrastructure closely on that in DeclPrinterTest.cpp and StmtPrinterTest.cpp. I figured the general framework could come in useful to people adding new type printing tests in the future.

I'm happy to trim it down a bit (e.g. remove unused methods and such), but in terms of the general approach, I feel like there's value in being consistent with the other printer tests?

Ah, sorry, had meant to ask if it'd be inspired by something else - if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

That had occurred to me as well. I'm happy to try!

Would it perhaps make sense to do that in a follow-up patch?

if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

That had occurred to me as well. I'm happy to try!

Would it perhaps make sense to do that in a follow-up patch?

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

Alternatively: Types are decls, perhaps this test should go in the existing DeclPrinterTest?

nridge planned changes to this revision.Jun 28 2021, 11:26 PM

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

Sounds good, will give that a try.

Alternatively: Types are decls, perhaps this test should go in the existing DeclPrinterTest?

Types often have corresponding decls (not always, e.g. BuiltinType), but they're distinct hierarchies (e.g. RecordType and RecordDecl can be thought of as "corresponding", but they're not related by inheritance, instead deriving from Type and Decl respectively). DeclPrinterTest exercises Decl::print(), while we want TypePrinterTest to exercise Type::print(), so I think a distinct file makes sense.

nridge updated this revision to Diff 356604.Jul 5 2021, 10:53 PM

Rebase on top of refactor patch

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

I posted the patch to generalize the AST printing test infrastructure at https://reviews.llvm.org/D105457.

This patch now depends on that one.

nridge updated this revision to Diff 357827.Sun, Jul 11, 7:15 PM

Rebase on top of refactor patch changes

@dblaikie now that the dependent patch has been merged and (presumably) stuck, do you think we can proceed with this one?

dblaikie accepted this revision.Mon, Jul 19, 9:43 AM

Sure, sounds good, thanks!

This revision is now accepted and ready to land.Mon, Jul 19, 9:43 AM
This revision was landed with ongoing or failed builds.Mon, Jul 19, 2:32 PM
This revision was automatically updated to reflect the committed changes.