[PATCH] D104619: [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 355103.
nridge added a comment.
Herald added a subscriber: mgorny.

add test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104619/new/

https://reviews.llvm.org/D104619

Files:
  clang/lib/AST/TypePrinter.cpp
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -0,0 +1,127 @@
+//===- unittests/AST/StmtPrinterTest.cpp --- Statement printer tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Stmt::printPretty() and related methods.
+//
+// Search this file for WRONG to see test cases that are producing something
+// completely wrong, invalid C++ or just misleading.
+//
+// These tests have a coding convention:
+// * statements to be printed should be contained within a function named 'A'
+//   unless it should have some special name (e.g., 'operator+');
+// * additional helper declarations are 'Z', 'Y', 'X' and so on.
+//
+//===--===//
+
+#include "ASTPrint.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallString.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ast_matchers;
+using namespace tooling;
+
+namespace {
+
+using PolicyAdjusterType =
+Optional>;
+
+static void PrintType(raw_ostream , const ASTContext *Context, QualType T,
+  PolicyAdjusterType PolicyAdjuster) {
+  assert(!T.isNull() && "Expected non-null Type");
+  PrintingPolicy Policy = Context->getPrintingPolicy();
+  if (PolicyAdjuster)
+(*PolicyAdjuster)(Policy);
+  T.print(Out, Policy);
+}
+
+class PrintMatch : public MatchFinder::MatchCallback {
+  SmallString<1024> Printed;
+  unsigned NumFoundTypes;
+  PolicyAdjusterType PolicyAdjuster;
+
+public:
+  explicit PrintMatch(PolicyAdjusterType PolicyAdjuster)
+  : NumFoundTypes(0), PolicyAdjuster(PolicyAdjuster) {}
+
+  void run(const MatchFinder::MatchResult ) override {
+const QualType *T = Result.Nodes.getNodeAs("id");
+if (!T)
+  return;
+NumFoundTypes++;
+if (NumFoundTypes > 1)
+  return;
+
+llvm::raw_svector_ostream Out(Printed);
+PrintType(Out, Result.Context, *T, PolicyAdjuster);
+  }
+
+  StringRef getPrinted() const { return Printed; }
+
+  unsigned getNumFoundTypes() const { return NumFoundTypes; }
+};
+
+::testing::AssertionResult
+PrintedTypeMatches(StringRef Code, const std::vector ,
+   const DeclarationMatcher ,
+   StringRef ExpectedPrinted,
+   PolicyAdjusterType PolicyAdjuster) {
+  PrintMatch Printer(PolicyAdjuster);
+  MatchFinder Finder;
+  Finder.addMatcher(NodeMatch, );
+  std::unique_ptr Factory =
+  newFrontendActionFactory();
+
+  if (!runToolOnCodeWithArgs(Factory->create(), Code, Args))
+return testing::AssertionFailure()
+   << "Parsing error in \"" << Code.str() << "\"";
+
+  if (Printer.getNumFoundTypes() == 0)
+return testing::AssertionFailure() << "Matcher didn't find any types";
+
+  if (Printer.getNumFoundTypes() > 1)
+return testing::AssertionFailure() << "Matcher should match only one type "
+  "(found "
+   << Printer.getNumFoundTypes() << ")";
+
+  if (Printer.getPrinted() != ExpectedPrinted)
+return ::testing::AssertionFailure()
+   << "Expected \"" << ExpectedPrinted.str()
+   << "\", "
+  "got \""
+   << Printer.getPrinted().str() << "\"";
+
+  return ::testing::AssertionSuccess();
+}
+
+} // unnamed namespace
+
+TEST(TypePrinter, TemplateId) {
+  std::string Code = R"cpp(
+namespace N {
+  template  struct Type {};
+  
+  template 
+  void Foo(const Type );
+}
+  )cpp";
+  auto Matcher = parmVarDecl(hasType(qualType().bind("id")));
+
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "const Type &",
+ PolicyAdjusterType([](PrintingPolicy ) {
+   Policy.FullyQualifiedName = false;
+ })));
+
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "const N::Type &",
+ PolicyAdjusterType([](PrintingPolicy ) {
+   Policy.FullyQualifiedName = true;
+ })));
+}
\ No newline at end of file
Index: clang/unittests/AST/CMakeLists.txt

[PATCH] D104619: [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D104619#2831956 , @nridge wrote:

> Thanks for having a look!
>
> In D104619#2831953 , @dblaikie 
> wrote:
>
>> 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104619/new/

https://reviews.llvm.org/D104619

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits