[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333932: [clang-doc] Implement reducer portion of the 
frontend framework (authored by juliehockett, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43341?vs=149500=149804#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43341

Files:
  clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
  clang-tools-extra/trunk/clang-doc/BitcodeReader.h
  clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp
  clang-tools-extra/trunk/clang-doc/BitcodeWriter.h
  clang-tools-extra/trunk/clang-doc/CMakeLists.txt
  clang-tools-extra/trunk/clang-doc/Representation.cpp
  clang-tools-extra/trunk/clang-doc/Representation.h
  clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/trunk/test/clang-doc/bc-comment.cpp
  clang-tools-extra/trunk/test/clang-doc/bc-namespace.cpp
  clang-tools-extra/trunk/test/clang-doc/bc-record.cpp

Index: clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
===
--- clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
+++ clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
@@ -18,7 +18,10 @@
 //
 //===--===//
 
+#include "BitcodeReader.h"
+#include "BitcodeWriter.h"
 #include "ClangDoc.h"
+#include "Representation.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
@@ -30,6 +33,7 @@
 #include "clang/Tooling/StandaloneExecution.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
@@ -54,14 +58,66 @@
  llvm::cl::desc("Dump mapper results to bitcode file."),
  llvm::cl::init(false), llvm::cl::cat(ClangDocCategory));
 
+static llvm::cl::opt DumpIntermediateResult(
+"dump-intermediate",
+llvm::cl::desc("Dump intermediate results to bitcode file."),
+llvm::cl::init(false), llvm::cl::cat(ClangDocCategory));
+
+enum OutputFormatTy {
+  yaml,
+};
+
+static llvm::cl::opt
+Format("format", llvm::cl::desc("Format for outputted docs."),
+   llvm::cl::values(clEnumVal(yaml, "Documentation in YAML format.")),
+   llvm::cl::init(yaml), llvm::cl::cat(ClangDocCategory));
+
 static llvm::cl::opt DoxygenOnly(
 "doxygen",
 llvm::cl::desc("Use only doxygen-style comments to generate docs."),
 llvm::cl::init(false), llvm::cl::cat(ClangDocCategory));
 
+bool CreateDirectory(const Twine , bool ClearDirectory = false) {
+  std::error_code OK;
+  llvm::SmallString<128> DocsRootPath;
+  if (ClearDirectory) {
+std::error_code RemoveStatus = llvm::sys::fs::remove_directories(DirName);
+if (RemoveStatus != OK) {
+  llvm::errs() << "Unable to remove existing documentation directory for "
+   << DirName << ".\n";
+  return true;
+}
+  }
+  std::error_code DirectoryStatus = llvm::sys::fs::create_directories(DirName);
+  if (DirectoryStatus != OK) {
+llvm::errs() << "Unable to create documentation directories.\n";
+return true;
+  }
+  return false;
+}
+
+bool DumpResultToFile(const Twine , const Twine ,
+  StringRef Buffer, bool ClearDirectory = false) {
+  std::error_code OK;
+  llvm::SmallString<128> IRRootPath;
+  llvm::sys::path::native(OutDirectory, IRRootPath);
+  llvm::sys::path::append(IRRootPath, DirName);
+  if (CreateDirectory(IRRootPath, ClearDirectory))
+return true;
+  llvm::sys::path::append(IRRootPath, FileName);
+  std::error_code OutErrorInfo;
+  llvm::raw_fd_ostream OS(IRRootPath, OutErrorInfo, llvm::sys::fs::F_None);
+  if (OutErrorInfo != OK) {
+llvm::errs() << "Error opening documentation file.\n";
+return true;
+  }
+  OS << Buffer;
+  OS.close();
+  return false;
+}
+
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
-  std::error_code OK;
 
   auto Exec = clang::tooling::createExecutorFromCommandLineArgs(
   argc, argv, ClangDocCategory);
@@ -80,34 +136,66 @@
 
   // Mapping phase
   llvm::outs() << "Mapping decls...\n";
-  auto Err = Exec->get()->execute(doc::newMapperActionFactory(
-  Exec->get()->getExecutionContext()),
-  ArgAdjuster);
-  if (Err)
+  auto Err = Exec->get()->execute(
+  doc::newMapperActionFactory(Exec->get()->getExecutionContext()),
+  ArgAdjuster);
+  if (Err) {
 llvm::errs() << toString(std::move(Err)) << "\n";
+return 1;
+  }
 
   if (DumpMapperResult) {
-Exec->get()->getToolResults()->forEachResult([&](StringRef Key,
- StringRef Value) {
-  SmallString<128> IRRootPath;
-  llvm::sys::path::native(OutDirectory, 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 149500.
juliehockett marked 8 inline comments as done.

https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,254 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump-intermediate -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/ACE81AFA6627B4CEF2B456FB6E1252925674AF7E.bc --dump | FileCheck %s --check-prefix CHECK-A
+// RUN: llvm-bcanalyzer %t/docs/bc/FC07BD34D5E77782C263FA97929EA8753740.bc --dump | FileCheck %s --check-prefix CHECK-B
+// RUN: llvm-bcanalyzer %t/docs/bc/1E3438A08BA22025C0B46289FF0686F92C8924C5.bc --dump | FileCheck %s --check-prefix CHECK-BC
+// RUN: llvm-bcanalyzer %t/docs/bc/06B5F6A19BA9F6A832E127C9968282B94619B210.bc --dump | FileCheck %s --check-prefix CHECK-C
+// RUN: llvm-bcanalyzer %t/docs/bc/0921737541208B8FA9BB42B60F78AC1D779AA054.bc --dump | FileCheck %s --check-prefix CHECK-D
+// RUN: llvm-bcanalyzer %t/docs/bc/289584A8E0FF4178A794622A547AA622503967A1.bc --dump | FileCheck %s --check-prefix CHECK-E
+// RUN: llvm-bcanalyzer %t/docs/bc/DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4.bc --dump | FileCheck %s --check-prefix CHECK-ECON
+// RUN: llvm-bcanalyzer %t/docs/bc/BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17.bc --dump | FileCheck %s --check-prefix CHECK-EDES
+// RUN: llvm-bcanalyzer %t/docs/bc/E3B54702FABFF4037025BA194FC27C47006330B5.bc --dump | FileCheck %s --check-prefix CHECK-F
+// RUN: llvm-bcanalyzer %t/docs/bc/B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E.bc --dump | FileCheck %s --check-prefix CHECK-H
+// RUN: llvm-bcanalyzer %t/docs/bc/6BA1EE2B3DAEACF6E4306F10AF44908F4807927C.bc --dump | FileCheck %s --check-prefix CHECK-I
+// RUN: llvm-bcanalyzer %t/docs/bc/5093D428CDC62096A67547BA52566E4FB9404EEE.bc --dump | FileCheck %s --check-prefix CHECK-PM
+// RUN: llvm-bcanalyzer %t/docs/bc/CA7C7935730B5EACD25F080E9C83FA087CCDC75E.bc --dump | FileCheck %s --check-prefix CHECK-X
+// RUN: llvm-bcanalyzer %t/docs/bc/641AB4A3D36399954ACDE29C7A8833032BF40472.bc --dump | FileCheck %s --check-prefix CHECK-Y
+
+void H() {
+  class I {};
+}
+// CHECK-H: 
+  // CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'H'
+  // CHECK-H-NEXT:  blob data = '{{.*}}'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'void'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+
+
+// CHECK-I: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = 'I'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT:  blob data = 'H'
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = '{{.*}}'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+
+union A { int X; int Y; };
+// CHECK-A: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'A'
+  // CHECK-A-NEXT:  blob data = '{{.*}}'
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'X'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'Y'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+
+enum B { X, Y };
+// CHECK-B: 
+  // CHECK-B-NEXT: 
+  // CHECK-B-NEXT:  blob data = 'B'
+  // CHECK-B-NEXT:  blob data = '{{.*}}'
+  // CHECK-B-NEXT:  blob data = 'X'
+  // CHECK-B-NEXT:  blob data = 'Y'
+// CHECK-B-NEXT: 
+
+enum class Bc { A, B };
+// CHECK-BC: 
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'Bc'
+  // CHECK-BC-NEXT:  blob data = '{{.*}}'
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'A'
+  // CHECK-BC-NEXT:  blob data = 'B'
+// CHECK-BC-NEXT: 
+
+struct C { int i; };
+// CHECK-C: 
+  // CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'C'
+  // CHECK-C-NEXT:  blob data = '{{.*}}'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'int'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+// CHECK-C-NEXT:  blob data = 'i'
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+
+class D {};
+// CHECK-D: 
+  // CHECK-D-NEXT: 
+  // CHECK-D-NEXT:  blob data = 'D'
+  // CHECK-D-NEXT:  blob data = '{{.*}}'
+  // CHECK-D-NEXT: 
+// 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lgtm with just a few more nits.




Comment at: clang-doc/BitcodeWriter.cpp:484
 
 #undef EMITINFO
 

Nit: `EMITINFO` is a bit confusing with `writeInfo`. Are they doing the same 
thing (`emit` sounds like `write`)? You might want to rename either of them. 
While you are here, it might also make sense to clear up the MACRO :) 



Comment at: clang-doc/Representation.cpp:57
+return llvm::make_error("Unexpected info type.\n",
+   std::error_code());
+  }

nit: `std::error_code()` is usually used to indicate no error. You could use 
`llvm::inconvertibleErrorCode()`.



Comment at: clang-doc/tool/ClangDocMain.cpp:181
+doc::writeInfo(I.get(), Buffer);
+  if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+return 1;

juliehockett wrote:
> ioeric wrote:
> > juliehockett wrote:
> > > ioeric wrote:
> > > > (Sorry that I might be missing context here.)
> > > > 
> > > > Could you please explain the incentive for dumping each info group to 
> > > > one bc file? If the key identifies a symbol (e.g. USR), wouldn't this 
> > > > result in a bitcode file being created for each symbol? This doesn't 
> > > > seem very scalable.  
> > > Yes, it would. This is mostly for debugging, since there's not really any 
> > > tools outside the clang system that would actually want/be able to use 
> > > this information.
> > Ok, is there any plan to convert intermediate result to final result and 
> > output in a more accessible format? If so, please add a `FIXME` somewhere 
> > just to be clearer.
> That's what the next patch set is (to generate YAML).
Sure. I think a `FIXME` should be in place if the feature is not already there.



Comment at: clang-doc/tool/ClangDocMain.cpp:68
+"format",
+llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));

nit: s/current option/default option/



Comment at: clang-doc/tool/ClangDocMain.cpp:68
+"format",
+llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));

ioeric wrote:
> nit: s/current option/default option/
consider using enum type options. Example: 
https://github.com/llvm-mirror/clang-tools-extra/blob/master/include-fixer/tool/ClangIncludeFixer.cpp#L85



Comment at: clang-doc/tool/ClangDocMain.cpp:178
+if (!Reduced)
+  llvm::errs() << llvm::toString(Reduced.takeError());
+

Shouldn't we `continue` to the next group if reduction goes wrong for the 
current group?


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-doc/tool/ClangDocMain.cpp:181
+doc::writeInfo(I.get(), Buffer);
+  if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+return 1;

ioeric wrote:
> juliehockett wrote:
> > ioeric wrote:
> > > (Sorry that I might be missing context here.)
> > > 
> > > Could you please explain the incentive for dumping each info group to one 
> > > bc file? If the key identifies a symbol (e.g. USR), wouldn't this result 
> > > in a bitcode file being created for each symbol? This doesn't seem very 
> > > scalable.  
> > Yes, it would. This is mostly for debugging, since there's not really any 
> > tools outside the clang system that would actually want/be able to use this 
> > information.
> Ok, is there any plan to convert intermediate result to final result and 
> output in a more accessible format? If so, please add a `FIXME` somewhere 
> just to be clearer.
That's what the next patch set is (to generate YAML).


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 149371.
juliehockett marked 13 inline comments as done.
juliehockett added a comment.

Fixing comments


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  docs/ReleaseNotes.rst
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,254 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump-intermediate -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/ACE81AFA6627B4CEF2B456FB6E1252925674AF7E.bc --dump | FileCheck %s --check-prefix CHECK-A
+// RUN: llvm-bcanalyzer %t/docs/bc/FC07BD34D5E77782C263FA97929EA8753740.bc --dump | FileCheck %s --check-prefix CHECK-B
+// RUN: llvm-bcanalyzer %t/docs/bc/1E3438A08BA22025C0B46289FF0686F92C8924C5.bc --dump | FileCheck %s --check-prefix CHECK-BC
+// RUN: llvm-bcanalyzer %t/docs/bc/06B5F6A19BA9F6A832E127C9968282B94619B210.bc --dump | FileCheck %s --check-prefix CHECK-C
+// RUN: llvm-bcanalyzer %t/docs/bc/0921737541208B8FA9BB42B60F78AC1D779AA054.bc --dump | FileCheck %s --check-prefix CHECK-D
+// RUN: llvm-bcanalyzer %t/docs/bc/289584A8E0FF4178A794622A547AA622503967A1.bc --dump | FileCheck %s --check-prefix CHECK-E
+// RUN: llvm-bcanalyzer %t/docs/bc/DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4.bc --dump | FileCheck %s --check-prefix CHECK-ECON
+// RUN: llvm-bcanalyzer %t/docs/bc/BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17.bc --dump | FileCheck %s --check-prefix CHECK-EDES
+// RUN: llvm-bcanalyzer %t/docs/bc/E3B54702FABFF4037025BA194FC27C47006330B5.bc --dump | FileCheck %s --check-prefix CHECK-F
+// RUN: llvm-bcanalyzer %t/docs/bc/B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E.bc --dump | FileCheck %s --check-prefix CHECK-H
+// RUN: llvm-bcanalyzer %t/docs/bc/6BA1EE2B3DAEACF6E4306F10AF44908F4807927C.bc --dump | FileCheck %s --check-prefix CHECK-I
+// RUN: llvm-bcanalyzer %t/docs/bc/5093D428CDC62096A67547BA52566E4FB9404EEE.bc --dump | FileCheck %s --check-prefix CHECK-PM
+// RUN: llvm-bcanalyzer %t/docs/bc/CA7C7935730B5EACD25F080E9C83FA087CCDC75E.bc --dump | FileCheck %s --check-prefix CHECK-X
+// RUN: llvm-bcanalyzer %t/docs/bc/641AB4A3D36399954ACDE29C7A8833032BF40472.bc --dump | FileCheck %s --check-prefix CHECK-Y
+
+void H() {
+  class I {};
+}
+// CHECK-H: 
+  // CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'H'
+  // CHECK-H-NEXT:  blob data = '{{.*}}'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'void'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+
+
+// CHECK-I: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = 'I'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT:  blob data = 'H'
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = '{{.*}}'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+
+union A { int X; int Y; };
+// CHECK-A: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'A'
+  // CHECK-A-NEXT:  blob data = '{{.*}}'
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'X'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'Y'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+
+enum B { X, Y };
+// CHECK-B: 
+  // CHECK-B-NEXT: 
+  // CHECK-B-NEXT:  blob data = 'B'
+  // CHECK-B-NEXT:  blob data = '{{.*}}'
+  // CHECK-B-NEXT:  blob data = 'X'
+  // CHECK-B-NEXT:  blob data = 'Y'
+// CHECK-B-NEXT: 
+
+enum class Bc { A, B };
+// CHECK-BC: 
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'Bc'
+  // CHECK-BC-NEXT:  blob data = '{{.*}}'
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'A'
+  // CHECK-BC-NEXT:  blob data = 'B'
+// CHECK-BC-NEXT: 
+
+struct C { int i; };
+// CHECK-C: 
+  // CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'C'
+  // CHECK-C-NEXT:  blob data = '{{.*}}'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'int'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+// CHECK-C-NEXT:  blob data = 'i'
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+
+class D {};
+// CHECK-D: 
+  // CHECK-D-NEXT: 
+  // CHECK-D-NEXT:  blob data = 'D'
+  

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-doc/BitcodeReader.cpp:553
+
+#define READINFO(INFO) 
\
+  {
\

Convert this to a template function?



Comment at: clang-doc/Reducer.cpp:19
+
+std::unique_ptr reduceInfos(std::vector> ) {
+  return mergeInfos(Values);

Now that merging is implemented in the info library, `reduceInfos` doesn't seem 
to be necessary. 

 I'd suggest moving `writeInfo` closer to the bitcode writer library and get 
rid of the Reducer.h/cpp. 



Comment at: clang-doc/Reducer.cpp:23
+
+bool writeInfo(Info *I, SmallVectorImpl ) {
+  llvm::BitstreamWriter Stream(Buffer);

I think a more canonical form to pass in an output buffer is via 
`llvm::raw_ostream`, and then callers can use `llvm::raw_string_ostream`.



Comment at: clang-doc/Representation.cpp:30
+
+#define ASSERT_MERGEABLE   
\
+  assert(IT == Other.IT && (USR == EmptySID || USR == Other.USR))

Macros are generally discouraged. Could you make this a function in `Info` e.g. 
`Info::mergeable(const Info )`. And sub-classes can simply can 
`asssert(mergeable(Other))`.



Comment at: clang-doc/Representation.cpp:59
+  case InfoType::IT_default:
+llvm::errs() << "Unexpected info type in index.\n";
+return nullptr;

Use `llvm::Expected>` (e.g. 
`llvm::make_error(...)`) for error handling and let callers decide 
how to handle the error (e.g. `llvm::errs() << llvm::toString(Err)`).



Comment at: clang-doc/Representation.h:146
+protected:
+  bool mergeBase(Info &);
 };

juliehockett wrote:
> ioeric wrote:
> > It's a bit awkward that users have to dispatch from info types to the 
> > corresponding `merge` function (as in `Reducer.cpp`). I think it would make 
> > users' life easier if the library handles the dispatching.
> > 
> > I wonder if something like the following would be better:
> > ```
> > struct Info {
> > std::unique_ptr merge(const Indo& LHS, const Info& RHS);
> > };
> > // A standalone function.
> > std::unique_ptr mergeInfo(const Info , const Info& RHS) {
> >   // merge base info.
> >   ...
> >   // merge subclass infos.
> >   assert(LHS.IT == RHS.IT); // or return nullptr
> >   switch (LHS.IT) {
> >... 
> > return Namespace::merge(LHS, RHS);
> >   } 
> > }
> > 
> > struct NamespaceInfo : public Info {
> >   std::unique_ptr merge(LHS, RHS);
> > };
> > 
> > ```
> > 
> > The unhandled switch case warning in compiler would help you catch 
> > unimplemented `merge` when new info types are added.
> Sort of addressed in this update. There's an issue with where we allocate the 
> return pointer, because we need to know the type of info at allocation time 
> -- let me know if what's here now is too far off of what you were suggesting.
Thanks! 

I thin what you have is also fine, as long as it's easy to maintain when adding 
new info types. 



Comment at: clang-doc/Representation.h:216
 
+// A standalone function to call to merge a vector of infos into one.
+std::unique_ptr mergeInfos(std::vector> );

Please elaborate a bit on the contract e.g. all values should have the same 
type; otherweise ...



Comment at: clang-doc/Representation.h:217
+// A standalone function to call to merge a vector of infos into one.
+std::unique_ptr mergeInfos(std::vector> );
+

nit: s/Value/Values/ or Infos



Comment at: clang-doc/tool/ClangDocMain.cpp:181
+doc::writeInfo(I.get(), Buffer);
+  if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+return 1;

juliehockett wrote:
> ioeric wrote:
> > (Sorry that I might be missing context here.)
> > 
> > Could you please explain the incentive for dumping each info group to one 
> > bc file? If the key identifies a symbol (e.g. USR), wouldn't this result in 
> > a bitcode file being created for each symbol? This doesn't seem very 
> > scalable.  
> Yes, it would. This is mostly for debugging, since there's not really any 
> tools outside the clang system that would actually want/be able to use this 
> information.
Ok, is there any plan to convert intermediate result to final result and output 
in a more accessible format? If so, please add a `FIXME` somewhere just to be 
clearer.



Comment at: clang-doc/tool/ClangDocMain.cpp:176
+Group.getValue().clear();
+Group.getValue().emplace_back(std::move(Reduced));
+

Rather then replace the values in `MapOutput`, it would probably be clearer if 
you store the reduced infos into a different container e.g. `Reduced`.



Comment at: docs/ReleaseNotes.rst:61
 
 - New module `abseil` for checks 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-doc/Representation.h:138
+  SymbolID USR =
+  SymbolID(); // Unique identifier for the decl described by this Info.
+  const InfoType IT = InfoType::IT_default; // InfoType of this particular 
Info.

ioeric wrote:
> Shouldn't USR be `SymbolID()` by default?
It actually is garbage by default -- doing this zeroed it out.



Comment at: clang-doc/Representation.h:146
+protected:
+  bool mergeBase(Info &);
 };

ioeric wrote:
> It's a bit awkward that users have to dispatch from info types to the 
> corresponding `merge` function (as in `Reducer.cpp`). I think it would make 
> users' life easier if the library handles the dispatching.
> 
> I wonder if something like the following would be better:
> ```
> struct Info {
> std::unique_ptr merge(const Indo& LHS, const Info& RHS);
> };
> // A standalone function.
> std::unique_ptr mergeInfo(const Info , const Info& RHS) {
>   // merge base info.
>   ...
>   // merge subclass infos.
>   assert(LHS.IT == RHS.IT); // or return nullptr
>   switch (LHS.IT) {
>... 
> return Namespace::merge(LHS, RHS);
>   } 
> }
> 
> struct NamespaceInfo : public Info {
>   std::unique_ptr merge(LHS, RHS);
> };
> 
> ```
> 
> The unhandled switch case warning in compiler would help you catch 
> unimplemented `merge` when new info types are added.
Sort of addressed in this update. There's an issue with where we allocate the 
return pointer, because we need to know the type of info at allocation time -- 
let me know if what's here now is too far off of what you were suggesting.



Comment at: clang-doc/tool/ClangDocMain.cpp:181
+doc::writeInfo(I.get(), Buffer);
+  if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+return 1;

ioeric wrote:
> (Sorry that I might be missing context here.)
> 
> Could you please explain the incentive for dumping each info group to one bc 
> file? If the key identifies a symbol (e.g. USR), wouldn't this result in a 
> bitcode file being created for each symbol? This doesn't seem very scalable.  
Yes, it would. This is mostly for debugging, since there's not really any tools 
outside the clang system that would actually want/be able to use this 
information.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 148678.
juliehockett marked 11 inline comments as done.
juliehockett added a comment.

Reworking the reducer interface a bit to address comments.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  docs/ReleaseNotes.rst
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,254 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump-intermediate -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/ACE81AFA6627B4CEF2B456FB6E1252925674AF7E.bc --dump | FileCheck %s --check-prefix CHECK-A
+// RUN: llvm-bcanalyzer %t/docs/bc/FC07BD34D5E77782C263FA97929EA8753740.bc --dump | FileCheck %s --check-prefix CHECK-B
+// RUN: llvm-bcanalyzer %t/docs/bc/1E3438A08BA22025C0B46289FF0686F92C8924C5.bc --dump | FileCheck %s --check-prefix CHECK-BC
+// RUN: llvm-bcanalyzer %t/docs/bc/06B5F6A19BA9F6A832E127C9968282B94619B210.bc --dump | FileCheck %s --check-prefix CHECK-C
+// RUN: llvm-bcanalyzer %t/docs/bc/0921737541208B8FA9BB42B60F78AC1D779AA054.bc --dump | FileCheck %s --check-prefix CHECK-D
+// RUN: llvm-bcanalyzer %t/docs/bc/289584A8E0FF4178A794622A547AA622503967A1.bc --dump | FileCheck %s --check-prefix CHECK-E
+// RUN: llvm-bcanalyzer %t/docs/bc/DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4.bc --dump | FileCheck %s --check-prefix CHECK-ECON
+// RUN: llvm-bcanalyzer %t/docs/bc/BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17.bc --dump | FileCheck %s --check-prefix CHECK-EDES
+// RUN: llvm-bcanalyzer %t/docs/bc/E3B54702FABFF4037025BA194FC27C47006330B5.bc --dump | FileCheck %s --check-prefix CHECK-F
+// RUN: llvm-bcanalyzer %t/docs/bc/B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E.bc --dump | FileCheck %s --check-prefix CHECK-H
+// RUN: llvm-bcanalyzer %t/docs/bc/6BA1EE2B3DAEACF6E4306F10AF44908F4807927C.bc --dump | FileCheck %s --check-prefix CHECK-I
+// RUN: llvm-bcanalyzer %t/docs/bc/5093D428CDC62096A67547BA52566E4FB9404EEE.bc --dump | FileCheck %s --check-prefix CHECK-PM
+// RUN: llvm-bcanalyzer %t/docs/bc/CA7C7935730B5EACD25F080E9C83FA087CCDC75E.bc --dump | FileCheck %s --check-prefix CHECK-X
+// RUN: llvm-bcanalyzer %t/docs/bc/641AB4A3D36399954ACDE29C7A8833032BF40472.bc --dump | FileCheck %s --check-prefix CHECK-Y
+
+void H() {
+  class I {};
+}
+// CHECK-H: 
+  // CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'H'
+  // CHECK-H-NEXT:  blob data = '{{.*}}'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'void'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+
+
+// CHECK-I: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = 'I'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT:  blob data = 'H'
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = '{{.*}}'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+
+union A { int X; int Y; };
+// CHECK-A: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'A'
+  // CHECK-A-NEXT:  blob data = '{{.*}}'
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'X'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'Y'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+
+enum B { X, Y };
+// CHECK-B: 
+  // CHECK-B-NEXT: 
+  // CHECK-B-NEXT:  blob data = 'B'
+  // CHECK-B-NEXT:  blob data = '{{.*}}'
+  // CHECK-B-NEXT:  blob data = 'X'
+  // CHECK-B-NEXT:  blob data = 'Y'
+// CHECK-B-NEXT: 
+
+enum class Bc { A, B };
+// CHECK-BC: 
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'Bc'
+  // CHECK-BC-NEXT:  blob data = '{{.*}}'
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'A'
+  // CHECK-BC-NEXT:  blob data = 'B'
+// CHECK-BC-NEXT: 
+
+struct C { int i; };
+// CHECK-C: 
+  // CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'C'
+  // CHECK-C-NEXT:  blob data = '{{.*}}'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'int'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+// CHECK-C-NEXT:  blob data = 'i'
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

The reduce logic seems to be in a good shape. Some nits and questions inlined.




Comment at: clang-doc/Reducer.cpp:19
+
+#define REDUCE(INFO)   
\
+  {
\

It seems that you could use a template function? 



Comment at: clang-doc/Reducer.cpp:24
+for (auto  : Values) {   
\
+  if (!Tmp->merge(std::move(*static_cast(I.get()   
\
+return nullptr;
\

Do we really want to give up all infos when one bad info/merge is present?



Comment at: clang-doc/Representation.h:138
+  SymbolID USR =
+  SymbolID(); // Unique identifier for the decl described by this Info.
+  const InfoType IT = InfoType::IT_default; // InfoType of this particular 
Info.

Shouldn't USR be `SymbolID()` by default?



Comment at: clang-doc/Representation.h:146
+protected:
+  bool mergeBase(Info &);
 };

It's a bit awkward that users have to dispatch from info types to the 
corresponding `merge` function (as in `Reducer.cpp`). I think it would make 
users' life easier if the library handles the dispatching.

I wonder if something like the following would be better:
```
struct Info {
std::unique_ptr merge(const Indo& LHS, const Info& RHS);
};
// A standalone function.
std::unique_ptr mergeInfo(const Info , const Info& RHS) {
  // merge base info.
  ...
  // merge subclass infos.
  assert(LHS.IT == RHS.IT); // or return nullptr
  switch (LHS.IT) {
   ... 
return Namespace::merge(LHS, RHS);
  } 
}

struct NamespaceInfo : public Info {
  std::unique_ptr merge(LHS, RHS);
};

```

The unhandled switch case warning in compiler would help you catch 
unimplemented `merge` when new info types are added.



Comment at: clang-doc/tool/ClangDocMain.cpp:60
 
+static llvm::cl::opt
+DumpResult("dump",

If this only dumps the intermediate results, should we call it something like 
`dump-intermediate` instead, to avoid confusion with final results?



Comment at: clang-doc/tool/ClangDocMain.cpp:148
+});
+return Err;
+  }

Print an error message on error?



Comment at: clang-doc/tool/ClangDocMain.cpp:154
+  llvm::StringMap> MapOutput;
+  Exec->get()->getToolResults()->forEachResult([&](StringRef Key,
+   StringRef Value) {

Could you add a brief comment explaining what key and value are?



Comment at: clang-doc/tool/ClangDocMain.cpp:161
+for (auto  : Infos) {
+  llvm::errs() << "COLLECT: " << llvm::toHex(llvm::toStringRef(I->USR))
+   << ": " << I->Name << "\n";

nit: remove debug logging?



Comment at: clang-doc/tool/ClangDocMain.cpp:170
+  // Reducing phase
+  llvm::outs() << "Reducing infos...\n";
+  for (auto  : MapOutput) {

nit: maybe also output the number of entries collected in the map?



Comment at: clang-doc/tool/ClangDocMain.cpp:181
+doc::writeInfo(I.get(), Buffer);
+  if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+return 1;

(Sorry that I might be missing context here.)

Could you please explain the incentive for dumping each info group to one bc 
file? If the key identifies a symbol (e.g. USR), wouldn't this result in a 
bitcode file being created for each symbol? This doesn't seem very scalable.  


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

ping


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

Pingany more thoughts?


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

In https://reviews.llvm.org/D43341#1093117, @juliehockett wrote:

> This will break things in clang-tools-extra without 
> https://reviews.llvm.org/D46615, so I'm going to hold off landing this until 
> that goes through


Oops wrong patch disregard


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

This will break things in clang-tools-extra without 
https://reviews.llvm.org/D46615, so I'm going to hold off landing this until 
that goes through


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-doc/Representation.cpp:53
+  move(Namespace, std::move(Other.Namespace));
+  extend(Description, std::move(Other.Description));
+  return true;

sammccall wrote:
> is plain concatenation of comments what you want?
Yes, in this case, as they aren't comment strings but vectors of comment infos. 


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 144948.
juliehockett marked 17 inline comments as done.
juliehockett added a comment.

Cleaning up and clarifying the merging process, and addressing comments


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  docs/ReleaseNotes.rst
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,254 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/ACE81AFA6627B4CEF2B456FB6E1252925674AF7E.bc --dump | FileCheck %s --check-prefix CHECK-A
+// RUN: llvm-bcanalyzer %t/docs/bc/FC07BD34D5E77782C263FA97929EA8753740.bc --dump | FileCheck %s --check-prefix CHECK-B
+// RUN: llvm-bcanalyzer %t/docs/bc/1E3438A08BA22025C0B46289FF0686F92C8924C5.bc --dump | FileCheck %s --check-prefix CHECK-BC
+// RUN: llvm-bcanalyzer %t/docs/bc/06B5F6A19BA9F6A832E127C9968282B94619B210.bc --dump | FileCheck %s --check-prefix CHECK-C
+// RUN: llvm-bcanalyzer %t/docs/bc/0921737541208B8FA9BB42B60F78AC1D779AA054.bc --dump | FileCheck %s --check-prefix CHECK-D
+// RUN: llvm-bcanalyzer %t/docs/bc/289584A8E0FF4178A794622A547AA622503967A1.bc --dump | FileCheck %s --check-prefix CHECK-E
+// RUN: llvm-bcanalyzer %t/docs/bc/DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4.bc --dump | FileCheck %s --check-prefix CHECK-ECON
+// RUN: llvm-bcanalyzer %t/docs/bc/BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17.bc --dump | FileCheck %s --check-prefix CHECK-EDES
+// RUN: llvm-bcanalyzer %t/docs/bc/E3B54702FABFF4037025BA194FC27C47006330B5.bc --dump | FileCheck %s --check-prefix CHECK-F
+// RUN: llvm-bcanalyzer %t/docs/bc/B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E.bc --dump | FileCheck %s --check-prefix CHECK-H
+// RUN: llvm-bcanalyzer %t/docs/bc/E1F2BCFC40AE6C36A65696CDCCA05B948F7B4F93.bc --dump | FileCheck %s --check-prefix CHECK-I
+// RUN: llvm-bcanalyzer %t/docs/bc/5093D428CDC62096A67547BA52566E4FB9404EEE.bc --dump | FileCheck %s --check-prefix CHECK-PM
+// RUN: llvm-bcanalyzer %t/docs/bc/CA7C7935730B5EACD25F080E9C83FA087CCDC75E.bc --dump | FileCheck %s --check-prefix CHECK-X
+// RUN: llvm-bcanalyzer %t/docs/bc/641AB4A3D36399954ACDE29C7A8833032BF40472.bc --dump | FileCheck %s --check-prefix CHECK-Y
+
+void H() {
+  class I {};
+}
+// CHECK-H: 
+  // CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'H'
+  // CHECK-H-NEXT:  blob data = '{{.*}}'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT:  blob data = 'void'
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+  // CHECK-H-NEXT: 
+// CHECK-H-NEXT: 
+
+
+// CHECK-I: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = 'I'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT:  blob data = 'H'
+// CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+  // CHECK-I-NEXT: 
+  // CHECK-I-NEXT:  blob data = '{{.*}}'
+  // CHECK-I-NEXT: 
+// CHECK-I-NEXT: 
+
+union A { int X; int Y; };
+// CHECK-A: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'A'
+  // CHECK-A-NEXT:  blob data = '{{.*}}'
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'X'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'int'
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'Y'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+
+enum B { X, Y };
+// CHECK-B: 
+  // CHECK-B-NEXT: 
+  // CHECK-B-NEXT:  blob data = 'B'
+  // CHECK-B-NEXT:  blob data = '{{.*}}'
+  // CHECK-B-NEXT:  blob data = 'X'
+  // CHECK-B-NEXT:  blob data = 'Y'
+// CHECK-B-NEXT: 
+
+enum class Bc { A, B };
+// CHECK-BC: 
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'Bc'
+  // CHECK-BC-NEXT:  blob data = '{{.*}}'
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'A'
+  // CHECK-BC-NEXT:  blob data = 'B'
+// CHECK-BC-NEXT: 
+
+struct C { int i; };
+// CHECK-C: 
+  // CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'C'
+  // CHECK-C-NEXT:  blob data = '{{.*}}'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'int'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+// CHECK-C-NEXT:  blob data = 'i'
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-doc/Index.h:28
+// Abstract class representing an index of mapped info objects.
+class Index {
+public:

What is this interface for? It looks like none of the functions are ever called 
through the interface.

If the intent is to abstract away a MR framework, I don't think this achieves 
that - multimachine MR frameworks have their "index" distributed and probably 
won't have a "reduce" function you can call with these semantics.

Consider just moving the in-memory reduction to the main file (which isn't 
portable across MR abstractions anyway) and avoiding abstractions there:
```
StringMap>>  MapOutput;
// populate MapOutput as you currently do in inMemoryReduceResults
for (const auto  : MapOutput)
  Write(Group.first, reduceInfos(std::move(Group.second)));
```



Comment at: clang-doc/Reducer.cpp:18
+
+#define REDUCE(INFO, TYPE) 
\
+  {
\

TYPE is unused?



Comment at: clang-doc/Representation.cpp:1
+///===-- Representation.cpp - ClangDoc Representation ---*- C++ 
-*-===//
+//

this is important/subtle logic, and there are no comments (particularly about 
motivating how specific fields are merged)!



Comment at: clang-doc/Representation.cpp:15
+
+template  void assign(T , T ) {
+  if (L != R)

why this, rather than actual assignment? (comments!)



Comment at: clang-doc/Representation.cpp:20
+
+template  void move(T , T &) {
+  if (L != R)

why would you ever assign() rather than move()



Comment at: clang-doc/Representation.cpp:26
+template <>
+void move(llvm::SmallVectorImpl ,
+  llvm::SmallVectorImpl &) {

why this pattern of "assign if empty" for these types?



Comment at: clang-doc/Representation.cpp:39
+template 
+void extend(llvm::SmallVectorImpl , llvm::SmallVectorImpl &) {
+  std::move(R.begin(), R.end(), std::back_inserter(L));

what if there are duplicatel?



Comment at: clang-doc/Representation.cpp:53
+  move(Namespace, std::move(Other.Namespace));
+  extend(Description, std::move(Other.Description));
+  return true;

is plain concatenation of comments what you want?



Comment at: clang-doc/Representation.h:75
 
+  bool operator==(const Reference ) const {
+return USR == Other.USR && UnresolvedName == Other.UnresolvedName &&

consider `std::tie(A, B) == std::tie(Other.A, Other.B)`



Comment at: clang-doc/Representation.h:79
+  }
+  bool operator!=(const Reference ) const {
+return USR != Other.USR || UnresolvedName != Other.UnresolvedName ||

do you really need `!=`?
if so, prefer to define it as `!(*this == Other)` to avoid redundancy.



Comment at: clang-doc/Representation.h:159
+  Info(InfoType IT) : IT(IT) {}
+  Info(Info ) = delete;
+  Info(Info &) = default;

should this be a copy constructor? (reference should be const)



Comment at: clang-doc/Representation.h:162
+
+  bool merge(Info &);
 

So having this "overriding" that's not virtual seems like asking for trouble: 
if you accidentally deref a unique_ptr and compare it, you'll get the 
base behavior which should probably never be called directly.

Consider naming this `mergeBase` and making it protected.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

juliehockett wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > juliehockett wrote:
> > > > lebedev.ri wrote:
> > > > > Isn't that the opposite of what was that assert supposed to do?
> > > > > It would have been better to just `// disable` it, and add a `FIXME` 
> > > > > note.
> > > > I'm not sure I'm understanding you -- my understanding was that it 
> > > > existed to check that the record size was large enough.
> > > https://reviews.llvm.org/D41102?id=136520#inline-384087
> > > ```
> > > #ifndef NDEBUG // Don't want explicit dtor unless needed
> > > ~ClangDocBitcodeWriter() {
> > >   // Check that the static size is large-enough.
> > >   assert(Record.capacity() == BitCodeConstants::RecordSize);
> > > }
> > > #endif
> > > ```
> > > I.e. it checked that it still only had the static size, and did not 
> > > transform into normal `vector` with data stored on heap-allocated buffer.
> > Ok, let me use more words this time.
> > There are several storage container types with contiguous storage:
> > * `array` - fixed-size stack storage, size == capacity == compile-time 
> > constant
> > * `vector` - dynamic heap storage, size <= capacity
> > * `smallvector` - fixed-size stack storage, and if the data does not fit, 
> > it degrades into the `vector`.
> > 
> > But there is also one more option:
> > * `fixedvector` - which is a `array` - has only fixed-size stack storage 
> > (capacity == compile-time constant), but with vector-like interface, i.e. 
> > size <= capacity, and size can be changed at run-time.
> > 
> > In llvm, i'm not aware of existence of `llvm::fixedvector`, so that assert 
> > was ensuring that the `smallvector` behaved like the `fixedvector` - it 
> > only had the fixed-size stack storage, and 'did not have' the heap buffer.
> > 
> > This is a kinda ugly hack, but i think the current assert clearly does 
> > something different...
> > 
> > Does that make any sense?
> Ah, yes. I see what you're trying to do, but the issue with that is it 
> assumes that the record must be a certain size (or smaller). However, there 
> is no guarantee of the size of the record (i.e. there may and will be records 
> of variable size getting written), so it's not unreasonable to think that the 
> record may need to be resized to accommodate that.
> However, there is no guarantee of the size of the record (i.e. there may and 
> will be records of variable size getting written), so it's not unreasonable 
> to think that the record may need to be resized to accommodate that.

Yes, but *currently* that was correct assumption.
All variable-length records (well, blobs) are (were?) emitted directly, without 
using this intermediate `Record` container.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett marked 5 inline comments as done.
juliehockett added inline comments.



Comment at: clang-doc/BitcodeReader.h:59
+
+  void storeData(llvm::SmallVectorImpl , llvm::StringRef Blob);
+  void storeData(bool , llvm::StringRef Blob);

sammccall wrote:
> Comment for these like "decodes the blob part of a record" - or just rename 
> these `decodeBlob` or similar?
In general they decode the record abbrev of the field, which is a single blob 
in many, but not all, cases. I've changed it to `decodeRecord`, but is too 
similar to the above?



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

lebedev.ri wrote:
> lebedev.ri wrote:
> > juliehockett wrote:
> > > lebedev.ri wrote:
> > > > Isn't that the opposite of what was that assert supposed to do?
> > > > It would have been better to just `// disable` it, and add a `FIXME` 
> > > > note.
> > > I'm not sure I'm understanding you -- my understanding was that it 
> > > existed to check that the record size was large enough.
> > https://reviews.llvm.org/D41102?id=136520#inline-384087
> > ```
> > #ifndef NDEBUG // Don't want explicit dtor unless needed
> > ~ClangDocBitcodeWriter() {
> >   // Check that the static size is large-enough.
> >   assert(Record.capacity() == BitCodeConstants::RecordSize);
> > }
> > #endif
> > ```
> > I.e. it checked that it still only had the static size, and did not 
> > transform into normal `vector` with data stored on heap-allocated buffer.
> Ok, let me use more words this time.
> There are several storage container types with contiguous storage:
> * `array` - fixed-size stack storage, size == capacity == compile-time 
> constant
> * `vector` - dynamic heap storage, size <= capacity
> * `smallvector` - fixed-size stack storage, and if the data does not fit, it 
> degrades into the `vector`.
> 
> But there is also one more option:
> * `fixedvector` - which is a `array` - has only fixed-size stack storage 
> (capacity == compile-time constant), but with vector-like interface, i.e. 
> size <= capacity, and size can be changed at run-time.
> 
> In llvm, i'm not aware of existence of `llvm::fixedvector`, so that assert 
> was ensuring that the `smallvector` behaved like the `fixedvector` - it only 
> had the fixed-size stack storage, and 'did not have' the heap buffer.
> 
> This is a kinda ugly hack, but i think the current assert clearly does 
> something different...
> 
> Does that make any sense?
Ah, yes. I see what you're trying to do, but the issue with that is it assumes 
that the record must be a certain size (or smaller). However, there is no 
guarantee of the size of the record (i.e. there may and will be records of 
variable size getting written), so it's not unreasonable to think that the 
record may need to be resized to accommodate that.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 144011.
juliehockett marked 17 inline comments as done.
juliehockett added a comment.

Reorganizing and streamlining, particularly in decoupling the reader from the 
reduce process and redesigning a bit to allow for more flexible reducing. 
Currently implements an in-memory reducer, but could (theoretically) be 
extended.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Index.cpp
  clang-doc/Index.h
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  docs/ReleaseNotes.rst
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,183 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/ACE81AFA6627B4CEF2B456FB6E1252925674AF7E.bc --dump | FileCheck %s --check-prefix CHECK-A
+// RUN: llvm-bcanalyzer %t/docs/bc/FC07BD34D5E77782C263FA97929EA8753740.bc --dump | FileCheck %s --check-prefix CHECK-B
+// RUN: llvm-bcanalyzer %t/docs/bc/1E3438A08BA22025C0B46289FF0686F92C8924C5.bc --dump | FileCheck %s --check-prefix CHECK-BC
+// RUN: llvm-bcanalyzer %t/docs/bc/06B5F6A19BA9F6A832E127C9968282B94619B210.bc --dump | FileCheck %s --check-prefix CHECK-C
+// RUN: llvm-bcanalyzer %t/docs/bc/0921737541208B8FA9BB42B60F78AC1D779AA054.bc --dump | FileCheck %s --check-prefix CHECK-D
+// RUN: llvm-bcanalyzer %t/docs/bc/289584A8E0FF4178A794622A547AA622503967A1.bc --dump | FileCheck %s --check-prefix CHECK-E
+// RUN: llvm-bcanalyzer %t/docs/bc/DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4.bc --dump | FileCheck %s --check-prefix CHECK-ECON
+// RUN: llvm-bcanalyzer %t/docs/bc/BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17.bc --dump | FileCheck %s --check-prefix CHECK-EDES
+// RUN: llvm-bcanalyzer %t/docs/bc/E3B54702FABFF4037025BA194FC27C47006330B5.bc --dump | FileCheck %s --check-prefix CHECK-F
+// RUN: llvm-bcanalyzer %t/docs/bc/B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E.bc --dump | FileCheck %s --check-prefix CHECK-H
+// RUN: llvm-bcanalyzer %t/docs/bc/E81CE07BB3FCCC7A88D059FC33F4140DF74F63DA.bc --dump | FileCheck %s --check-prefix CHECK-I
+// RUN: llvm-bcanalyzer %t/docs/bc/5093D428CDC62096A67547BA52566E4FB9404EEE.bc --dump | FileCheck %s --check-prefix CHECK-PM
+// RUN: llvm-bcanalyzer %t/docs/bc/CA7C7935730B5EACD25F080E9C83FA087CCDC75E.bc --dump | FileCheck %s --check-prefix CHECK-X
+// RUN: llvm-bcanalyzer %t/docs/bc/641AB4A3D36399954ACDE29C7A8833032BF40472.bc --dump | FileCheck %s --check-prefix CHECK-Y
+
+union A { int X; int Y; };
+// CHECK-A: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT:  blob data = 'A'
+  // CHECK-A-NEXT:  blob data = '{{.*}}'
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'int'
+// CHECK-A-NEXT:  blob data = 'A::X'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT:  blob data = 'int'
+// CHECK-A-NEXT:  blob data = 'A::Y'
+// CHECK-A-NEXT: 
+  // CHECK-A-NEXT: 
+// CHECK-A-NEXT: 
+
+enum B { X, Y };
+// CHECK-B: 
+  // CHECK-B-NEXT: 
+  // CHECK-B-NEXT:  blob data = 'B'
+  // CHECK-B-NEXT:  blob data = '{{.*}}'
+  // CHECK-B-NEXT:  blob data = 'X'
+  // CHECK-B-NEXT:  blob data = 'Y'
+// CHECK-B-NEXT: 
+
+enum class Bc { A, B };
+// CHECK-BC: 
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'Bc'
+  // CHECK-BC-NEXT:  blob data = '{{.*}}'
+  // CHECK-BC-NEXT: 
+  // CHECK-BC-NEXT:  blob data = 'A'
+  // CHECK-BC-NEXT:  blob data = 'B'
+// CHECK-BC-NEXT: 
+
+struct C { int i; };
+// CHECK-C: 
+  // CHECK-C-NEXT: 
+  // CHECK-C-NEXT:  blob data = 'C'
+  // CHECK-C-NEXT:  blob data = '{{.*}}'
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT:  blob data = 'int'
+// CHECK-C-NEXT:  blob data = 'C::i'
+// CHECK-C-NEXT: 
+  // CHECK-C-NEXT: 
+// CHECK-C-NEXT: 
+
+class D {};
+// CHECK-D: 
+  // CHECK-D-NEXT: 
+  // CHECK-D-NEXT:  blob data = 'D'
+  // CHECK-D-NEXT:  blob data = '{{.*}}'
+  // CHECK-D-NEXT: 
+// CHECK-D-NEXT: 
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+// CHECK-E: 
+  // CHECK-E-NEXT: 
+  // CHECK-E-NEXT:  blob data = 'E'
+  // CHECK-E-NEXT:  blob data = '{{.*}}'
+  // CHECK-E-NEXT: 
+// CHECK-E-NEXT: 
+
+// CHECK-ECON: 
+  // CHECK-ECON-NEXT: 
+  // CHECK-ECON-NEXT:  blob data = 'E'
+  // CHECK-ECON-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-ECON-NEXT: 
+  // CHECK-ECON-NEXT:  blob data = 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, I didn't get through all the detail, but we should talk about the MR stuff 
today :)




Comment at: clang-doc/BitcodeReader.cpp:25
+void ClangDocBitcodeReader::storeData(SymbolID , llvm::StringRef Blob) {
+  assert(Record[0] == 20);
+  // First position in the record is the length of the following array, so we

why is this true?
i.e. why can't I produce a malformed bitcode file that causes this assertion to 
fail?
(My usual understanding is that a failed assert is meant to indicate a 
programming error, if it means something else here like "malformed file" you 
should probably document that if you're sure it's what you want)



Comment at: clang-doc/BitcodeReader.cpp:42
+  llvm::StringRef Blob) {
+  Field = (AccessSpecifier)Record[0];
+}

Similarly, what guarantees Record[0] is in-range for AccessSpecifier? casting 
an out-of-range integer to an enum results in UB.



Comment at: clang-doc/BitcodeReader.h:11
+// This file implements a reader for parsing the clang-doc internal
+// representation to LLVM bitcode. The reader takes in a stream of bits and
+// generates the set of infos that it represents.

nit: *from* LLVM bitcode, or am I missing something?



Comment at: clang-doc/BitcodeReader.h:29
+// Class to read bitstream into an InfoSet collection
+class ClangDocBitcodeReader {
+public:

I found the implementation here a bit hard to understand at first, mostly 
because the state/invariants/responsibilities of each method weren't obvious to 
me.
Left some suggestions below to document these a bit more.

However many of these seem to in practice only share the Record buffer. It 
seems making this an explicit parameter would let you make these free 
functions, hiding them from the header and avoiding any possibility of spooky 
side-channel interactions. Up to you of course.



Comment at: clang-doc/BitcodeReader.h:46
+
+  // Reading records
+  template  bool readRecord(unsigned ID, TInfo );

I think this needs some elaboration :-)
e.g. Populates Record with the decoded record, and then calls one of the 
`parseRecord` overloads.

In general the data flow between these methods seems a little obscured by 
having Record be a field rather than a parameter as ID/Blob are.
(Incidentally I don't think you gain anything here from the use of SmallVector 
over std::vector, which is easier to type, but it doesn't really matter)



Comment at: clang-doc/BitcodeReader.h:49
+
+  bool parseRecord(unsigned ID, llvm::StringRef Blob, unsigned VersionNo);
+  bool parseRecord(unsigned ID, llvm::StringRef Blob, NamespaceInfo );

comment for these methods e.g. Populates the appropriate field of the Info 
object based on a decoded record whose content is in Record.



Comment at: clang-doc/BitcodeReader.h:59
+
+  void storeData(llvm::SmallVectorImpl , llvm::StringRef Blob);
+  void storeData(bool , llvm::StringRef Blob);

Comment for these like "decodes the blob part of a record" - or just rename 
these `decodeBlob` or similar?



Comment at: clang-doc/Reducer.cpp:17
+
+std::unique_ptr mergeInfos(tooling::ToolResults *Results) {
+  std::unique_ptr UniqueInfos = llvm::make_unique();

A little bit of context to document here: the fact that ToolResults values are 
bit-encoded InfoSets describing each TU in the codebase. (I think?)

(Also, why not return InfoSet by value?)



Comment at: clang-doc/Reducer.cpp:23
+ClangDocBitcodeReader Reader(Stream);
+if (!Reader.readBitstream(*UniqueInfos)) {
+  Err = true;

I don't see where any actual merging is happening.
Say we process TU 1 which has a decl of foo() with a doc comment, and TU 2 
which defines foo(). Now we're going to have something like:
ToolResults = {
 "tu1": "...bitcode...foo().../*does foo*/",
 "tu2": "...bitcode...foo()...Foo.cpp:53",
}
the first readBitstream call is going to ultimately call 
StoreData(UniqueInfos->Functions), which appends a new function to the array
then the second readBitstream call will do the same thing, and you have an 
InfoSet with two copies of foo()

what am I missing?



Comment at: clang-doc/Reducer.h:21
+// Combine occurrences of the same info across translation units.
+std::unique_ptr mergeInfos(tooling::ToolResults *Results);
+

This doesn't quite look like the typical reduce pattern. (And there's no 
framework or guidance for MRs in libtooling so this isn't surprising or maybe 
even a problem.

When the mapper emits its values, it usually chooses the keys so that a 
non-trivial merge is only required among values with the same key. (e.g. USR 
here)
This does two things:
1. it makes it really easy to parallelize: after the mappers run in 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 140275.
juliehockett added a comment.

Fixing assert on vector size.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  docs/ReleaseNotes.rst
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,178 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+enum class Bc { A, B };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+class X {
+  class Y {};
+};
+
+void H() {
+  class I {};
+}
+
+// CHECK: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '~E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'H'
+  // CHECK-NEXT:  blob data = '3433664532ABFCC39301646A10E768C1882BF194'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'A'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::X'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::Y'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'C'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'C::i'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'D'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'F'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'X'
+  // CHECK-NEXT:  blob data = '641AB4A3D36399954ACDE29C7A8833032BF40472'
+  // CHECK-NEXT:  

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Isn't that the opposite of what was that assert supposed to do?
> > > It would have been better to just `// disable` it, and add a `FIXME` note.
> > I'm not sure I'm understanding you -- my understanding was that it existed 
> > to check that the record size was large enough.
> https://reviews.llvm.org/D41102?id=136520#inline-384087
> ```
> #ifndef NDEBUG // Don't want explicit dtor unless needed
> ~ClangDocBitcodeWriter() {
>   // Check that the static size is large-enough.
>   assert(Record.capacity() == BitCodeConstants::RecordSize);
> }
> #endif
> ```
> I.e. it checked that it still only had the static size, and did not transform 
> into normal `vector` with data stored on heap-allocated buffer.
Ok, let me use more words this time.
There are several storage container types with contiguous storage:
* `array` - fixed-size stack storage, size == capacity == compile-time constant
* `vector` - dynamic heap storage, size <= capacity
* `smallvector` - fixed-size stack storage, and if the data does not fit, it 
degrades into the `vector`.

But there is also one more option:
* `fixedvector` - which is a `array` - has only fixed-size stack storage 
(capacity == compile-time constant), but with vector-like interface, i.e. size 
<= capacity, and size can be changed at run-time.

In llvm, i'm not aware of existence of `llvm::fixedvector`, so that assert was 
ensuring that the `smallvector` behaved like the `fixedvector` - it only had 
the fixed-size stack storage, and 'did not have' the heap buffer.

This is a kinda ugly hack, but i think the current assert clearly does 
something different...

Does that make any sense?



Comment at: clang-doc/Representation.cpp:28
+  if (L.Namespace.empty())
+L.Namespace = std::move(R.Namespace);
+  std::move(R.Description.begin(), R.Description.end(),

This was changed, but no test changes followed.
Needs more tests. The test coverage is clearly bad.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

juliehockett wrote:
> lebedev.ri wrote:
> > Isn't that the opposite of what was that assert supposed to do?
> > It would have been better to just `// disable` it, and add a `FIXME` note.
> I'm not sure I'm understanding you -- my understanding was that it existed to 
> check that the record size was large enough.
https://reviews.llvm.org/D41102?id=136520#inline-384087
```
#ifndef NDEBUG // Don't want explicit dtor unless needed
~ClangDocBitcodeWriter() {
  // Check that the static size is large-enough.
  assert(Record.capacity() == BitCodeConstants::RecordSize);
}
#endif
```
I.e. it checked that it still only had the static size, and did not transform 
into normal `vector` with data stored on heap-allocated buffer.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-doc/BitcodeReader.cpp:27
+  assert(Record[0] == 20);
+  for (int I = 0, E = Record[0]; I < E; ++I)
+Field[I] = Record[I + 1];

lebedev.ri wrote:
> Ok, i don't understand what is going on here.
> Where is this `E` defined?
> This looks like `[0]` is the number of elements to read (always 20, sha1 
> blob?),
> and it copies Record[1]..Record[20] to Field[0]..Field[19] ?
> I think this can/should be rewritten clearer..
`Record[0]` holds the size of the array -- in this case, 20 -- in the bitstream 
representation of the array (see the array section [[ 
https://llvm.org/docs/BitCodeFormat.html#define-abbrev-encoding | here ]]). So, 
yes, it copies Record[1]...[20] to Field[0]...[19]. Not sure how that can be 
rewritten more clearly, but I'll add a clarifying comment!



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

lebedev.ri wrote:
> Isn't that the opposite of what was that assert supposed to do?
> It would have been better to just `// disable` it, and add a `FIXME` note.
I'm not sure I'm understanding you -- my understanding was that it existed to 
check that the record size was large enough.



Comment at: clang-doc/Reducer.cpp:18
+std::unique_ptr mergeInfos(tooling::ToolResults *Results) {
+  std::unique_ptr UniqueInfos = llvm::make_unique();
+  bool Err = false;

lebedev.ri wrote:
> I can see that you may `return nullptr;` in case of error here, thus it's 
> `std::unique_ptr<>`
> `InfoSet` internally is just a few `std::vector<>`s, so it should 
> `std::move()` efficiently.
> I'm wondering if `llvm::Optional` would work too?
Doesn't `llvm::Optional` imply that it *could* validly not exist though? This 
is trying to express here that that isn't a valid state.



Comment at: clang-doc/Representation.h:135
   Info() = default;
-  Info(Info &) : Description(std::move(Other.Description)) {}
-  virtual ~Info() = default;
+  Info(Info &)
+  : USR(Other.USR), Name(Other.Name), 
Namespace(std::move(Other.Namespace)),

lebedev.ri wrote:
> Why is the move constructor explicitly defined?
> Unless i'm missing something, the default one would do exactly the same.
> https://godbolt.org/g/XqsRuX
It's there (as is the CommentInfo one) because otherwise it was throwing 
errors. It appears that a copy constructor is called somewhere if this isn't 
specified.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 139972.
juliehockett marked 4 inline comments as done.
juliehockett added a comment.

Small fixes to address comments


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,178 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+enum class Bc { A, B };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+class X {
+  class Y {};
+};
+
+void H() {
+  class I {};
+}
+
+// CHECK: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '~E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'H'
+  // CHECK-NEXT:  blob data = '3433664532ABFCC39301646A10E768C1882BF194'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'A'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::X'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::Y'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'C'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'C::i'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'D'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'F'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'X'
+  // CHECK-NEXT:  blob data = 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It's surprisingly difficult to review this :/
Just by itself the logic is simple, but the amount of blocks/records/record 
types is kinda demoralizing.
I really hope it will be possible to somehow tablegen this later.




Comment at: clang-doc/BitcodeReader.cpp:363
+CommentInfo &
+ClangDocBitcodeReader::getCommentInfo(std::unique_ptr ) {
+  I->Children.emplace_back(llvm::make_unique());

Hmm, why does this function exist?
The only difference from the previous one is the `std::unique_ptr<>`.



Comment at: clang-doc/BitcodeReader.cpp:488
+  // Sniff for the signature.
+  if (Stream.Read(8) != 'D' || Stream.Read(8) != 'O' || Stream.Read(8) != 'C' 
||
+  Stream.Read(8) != 'S')

This signature should be in one place (header?), and then just used here and in 
`ClangDocBitcodeWriter::emitHeader()`



Comment at: clang-doc/BitcodeWriter.cpp:537
 
+void ClangDocBitcodeWriter::emitInfoSet(InfoSet ) {
+  for (const auto  : ISet.getNamespaceInfos()) emitBlock(I);

It should be `void ClangDocBitcodeWriter::emitInfoSet(const InfoSet ) {` 
then.



Comment at: clang-doc/BitcodeWriter.h:129
 // Check that the static size is large-enough.
 assert(Record.capacity() > BitCodeConstants::RecordSize);
   }

Isn't that the opposite of what was that assert supposed to do?
It would have been better to just `// disable` it, and add a `FIXME` note.



Comment at: clang-doc/tool/ClangDocMain.cpp:138
+llvm::outs() << "Writing intermediate results...\n";
+SmallString<4098> Buffer;
+llvm::BitstreamWriter Stream(Buffer);

`4096`?


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new tool in Release Notes and use //:doc:// to refer to its 
manual.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 139869.
juliehockett marked 13 inline comments as done.
juliehockett added a comment.

Addressing comments


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,178 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+enum class Bc { A, B };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+class X {
+  class Y {};
+};
+
+void H() {
+  class I {};
+}
+
+// CHECK: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '~E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'H'
+  // CHECK-NEXT:  blob data = '3433664532ABFCC39301646A10E768C1882BF194'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'A'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::X'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::Y'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'C'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'C::i'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'D'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'F'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'X'
+  // CHECK-NEXT:  blob data = '641AB4A3D36399954ACDE29C7A8833032BF40472'
+  // 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It's good to finally have the initial block firmly landed, congratulations.

Trying to review this...
Some initial thoughts.




Comment at: clang-doc/BitcodeReader.cpp:27
+  assert(Record[0] == 20);
+  for (int I = 0, E = Record[0]; I < E; ++I)
+Field[I] = Record[I + 1];

Ok, i don't understand what is going on here.
Where is this `E` defined?
This looks like `[0]` is the number of elements to read (always 20, sha1 blob?),
and it copies Record[1]..Record[20] to Field[0]..Field[19] ?
I think this can/should be rewritten clearer..



Comment at: clang-doc/BitcodeReader.cpp:19
+
+void ClangDocBitcodeReader::storeData(llvm::SmallString<4> ,
+  llvm::StringRef Blob) {

juliehockett wrote:
> lebedev.ri wrote:
> > I think all these `SmallString` can be one `llvm::SmallVectorImpl`?
> No, since there's not an implicit converter from 
> `llvm::SmallVectorImpl` to `StringRef`. I templatized it on the size 
> though, so it's only one function now.
Are you sure?
https://godbolt.org/g/wD1FKD

That isn't pretty, but i think it beats templating in this case..



Comment at: clang-doc/BitcodeReader.h:33
+
+  bool readBitstream(std::unique_ptr );
+

Similarly, i think this should be
```
bool readBitstream(InfoSet );
```



Comment at: clang-doc/BitcodeReader.h:44
+  template 
+  bool readBlockToInfo(unsigned ID, std::unique_ptr );
+

As far as i can see this should be
```
template 
bool readBlockToInfo(unsigned ID, InfoSet );
```




Comment at: clang-doc/BitcodeWriter.h:142
   void emitBlock(const CommentInfo );
+  void emitInfoSet(std::unique_ptr );
 

And here too, it does not make much sense to call it with empty 
`std::unique_ptr<>`, so maybe
```
void emitInfoSet(InfoSet );
```
?



Comment at: clang-doc/Reducer.cpp:18
+std::unique_ptr mergeInfos(tooling::ToolResults *Results) {
+  std::unique_ptr UniqueInfos = llvm::make_unique();
+  bool Err = false;

I can see that you may `return nullptr;` in case of error here, thus it's 
`std::unique_ptr<>`
`InfoSet` internally is just a few `std::vector<>`s, so it should `std::move()` 
efficiently.
I'm wondering if `llvm::Optional` would work too?



Comment at: clang-doc/Representation.cpp:19
+  SymbolID USR;
+  std::string HexString = fromHex(StringUSR);
+  std::copy(HexString.begin(), HexString.end(), USR.begin());

I though this was changed, and the non-stringified, original binary version of 
sha1 was emitted into bitcode?



Comment at: clang-doc/Representation.cpp:28
+  if (L.Namespace.empty())
+std::move(R.Namespace);
+  std::move(R.Description.begin(), R.Description.end(),

???
Where *to* is it moved?



Comment at: clang-doc/Representation.cpp:40
+
+static void mergeInfo(NamespaceInfo , NamespaceInfo ) {
+  mergeInfoBase(L, R);

All these `mergeInfo()`: the second param, `Info ` should probably be `Info 
&`.
(but not `mergeInfoBase()`/`mergeSymbolInfoBase()`)



Comment at: clang-doc/Representation.cpp:98
+else   
\
+  mergeInfo(X##s[R.first->second], I); 
\
+  }

Seeing how many `std::move()`'ing is happening in those `mergeInfo()`, i think 
you want to move `I`, not pass as reference.
Especially since it is already `&` in this `InfoSet::insert()` function.



Comment at: clang-doc/Representation.h:30
 
 using SymbolID = std::array;
+SymbolID StringToSymbol(llvm::StringRef StringUSR);

Please add a comment explaining that `SymbolID` is sha1, and not hex-string of 
sha1.



Comment at: clang-doc/Representation.h:135
   Info() = default;
-  Info(Info &) : Description(std::move(Other.Description)) {}
-  virtual ~Info() = default;
+  Info(Info &)
+  : USR(Other.USR), Name(Other.Name), 
Namespace(std::move(Other.Namespace)),

Why is the move constructor explicitly defined?
Unless i'm missing something, the default one would do exactly the same.
https://godbolt.org/g/XqsRuX



Comment at: clang-doc/tool/ClangDocMain.cpp:130
+llvm::outs() << "Writing intermediate results...\n";
+SmallString<2048> Buffer;
+llvm::BitstreamWriter Stream(Buffer);

That is the same small-size used in `static std::string serialize(T )`.
Some statistic is needed, bu i think this one can be bumped somewhat right away.




Comment at: clang-doc/tool/ClangDocMain.cpp:141
+  llvm::errs() << "Unable to create documentation directories.\n";
+  return 1;
+}

This shares some code with `if(DumpMapperResult)`.
Perhaps you could refactor it 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 139644.
juliehockett added a comment.

Rebasing and updating.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,178 @@
+// This test requires Linux due to the system-dependent USR for the
+// inner class in function H.
+// REQUIRES: system-linux
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+enum class Bc { A, B };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+class X {
+  class Y {};
+};
+
+void H() {
+  class I {};
+}
+
+// CHECK: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '~E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'H'
+  // CHECK-NEXT:  blob data = '3433664532ABFCC39301646A10E768C1882BF194'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'A'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::X'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::Y'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'C'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'C::i'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'D'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'F'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'X'
+  // CHECK-NEXT:  blob data = '641AB4A3D36399954ACDE29C7A8833032BF40472'
+  // CHECK-NEXT:  blob data = '{{.*}}'
+  // 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Once the base differential firmly lands, could you please rebase this so the 
review could continue?


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-doc/BitcodeReader.cpp:19
+
+void ClangDocBitcodeReader::storeData(llvm::SmallString<4> ,
+  llvm::StringRef Blob) {

lebedev.ri wrote:
> I think all these `SmallString` can be one `llvm::SmallVectorImpl`?
No, since there's not an implicit converter from `llvm::SmallVectorImpl` 
to `StringRef`. I templatized it on the size though, so it's only one function 
now.



Comment at: clang-doc/Representation.h:193
+ private:
+  void resolveReferences(llvm::SmallVector ,
+ Reference );

lebedev.ri wrote:
> Similarly, i think those should be `SmallVectorImpl` (i assume those are 
> output params, too?)
Yup -- the pointer inside gets set.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 137245.
juliehockett marked 8 inline comments as done.
juliehockett added a comment.

Adding in support for mapper tests and addressing comments.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/bc-comment.cpp
  test/clang-doc/bc-namespace.cpp
  test/clang-doc/bc-record.cpp

Index: test/clang-doc/bc-record.cpp
===
--- /dev/null
+++ test/clang-doc/bc-record.cpp
@@ -0,0 +1,168 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+enum class Bc { A, B };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+class X {
+  class Y {};
+};
+
+void H() {
+  class I {};
+}
+
+// CHECK: 
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'DEB4AC1CD9253CD9EF7FBE6BCAC506D77984ABD4'
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'BD2BDEBD423F80BACCEA75DE6D6622D355FC2D17'
+  // CHECK-NEXT:  blob data = '~E'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '5093D428CDC62096A67547BA52566E4FB9404EEE'
+  // CHECK-NEXT:  blob data = 'ProtectedMethod'
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'B6AC4C5C9F2EA3F2B3ECE1A33D349F4EE502B24E'
+  // CHECK-NEXT:  blob data = 'H'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'void'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'ACE81AFA6627B4CEF2B456FB6E1252925674AF7E'
+  // CHECK-NEXT:  blob data = 'A'
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::X'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'A::Y'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '06B5F6A19BA9F6A832E127C9968282B94619B210'
+  // CHECK-NEXT:  blob data = 'C'
+  // CHECK-NEXT: 
+// CHECK-NEXT:  blob data = 'int'
+// CHECK-NEXT:  blob data = 'C::i'
+// CHECK-NEXT: 
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+  // CHECK-NEXT:  blob data = 'D'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = 'E'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'E3B54702FABFF4037025BA194FC27C47006330B5'
+  // CHECK-NEXT:  blob data = 'F'
+  // CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '289584A8E0FF4178A794622A547AA622503967A1'
+  // CHECK-NEXT:  blob data = '0921737541208B8FA9BB42B60F78AC1D779AA054'
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = 'CA7C7935730B5EACD25F080E9C83FA087CCDC75E'
+  // CHECK-NEXT:  blob data = 'X'
+  // CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+  // CHECK-NEXT:  blob data = '641AB4A3D36399954ACDE29C7A8833032BF40472'
+  // CHECK-NEXT:  blob data = 'Y'
+  // CHECK-NEXT:  blob data = 'CA7C7935730B5EACD25F080E9C83FA087CCDC75E'
+  // CHECK-NEXT: 
+// 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this!

Finally trying to review this...
I must say i'm **really** not fond of the test 'changes'.

But some initial comments added:




Comment at: clang-doc/BitcodeReader.cpp:19
+
+void ClangDocBitcodeReader::storeData(llvm::SmallString<4> ,
+  llvm::StringRef Blob) {

I think all these `SmallString` can be one `llvm::SmallVectorImpl`?



Comment at: clang-doc/BitcodeReader.cpp:454
+for (const auto  : (*BlockInfo).getBlockInfo(I)->RecordNames)
+  RecordNames[N.first] = N.second;
+  }

`RecordNames` is basically not kept between the reuses of 
`ClangDocBitcodeReader`, but is also not actually properly gc'd.
Also, this uses `BI_FIRST` / `BI_LAST`, which means the versioning is actually 
absolutely required...
Also, `RecordNames` isn't actually used anywhere later in this code.

So, Is that needed for the next patches? Or why read this at all?





Comment at: clang-doc/BitcodeReader.h:36
+
+  bool readBitstream(llvm::SmallString<2048> Bits,
+ std::unique_ptr );

`Bits` is not an output parameter, but just a const input parameter?
I think it would be cleaner to use `StringRef`, to avoid depending on the 
actual size of the small-size.



Comment at: clang-doc/BitcodeReader.h:46
+  bool readBlockInfoBlock(llvm::BitstreamCursor );
+  template 
+  bool readBlockToInfo(llvm::BitstreamCursor , unsigned ID,

Please separate those tree functions and this template function with one 
newline.



Comment at: clang-doc/BitcodeReader.h:48
+  bool readBlockToInfo(llvm::BitstreamCursor , unsigned ID,
+   std::unique_ptr , T &);
+

 `T &` looks super vague.
`T` is `{something}Info`, which is inherited from `Info` base-class.
Maybe something like `InfoT &` ?



Comment at: clang-doc/Reducer.cpp:19
+  std::unique_ptr UniqueInfos = llvm::make_unique();
+  doc::ClangDocBitcodeReader Reader;
+  bool Err = false;

As far as i can tell, `Reader` isn't required to be kept out here.
It seems it's not used to retain any internal state.
The only obvious reason to keep it, is to avoid de-allocating and then 
re-allocating the memory each time.

I'm wondering if it would be better to move it into the lambda.
Also, why is it prefixed with the `doc::`? We are in that namespace already.

Then, you will also be able to get rid of `llvm::BitstreamCursor ` 
params in the `ClangDocBitcodeReader` member functions, like with 
`ClangDocBitcodeWriter`.



Comment at: clang-doc/Representation.h:193
+ private:
+  void resolveReferences(llvm::SmallVector ,
+ Reference );

Similarly, i think those should be `SmallVectorImpl` (i assume those are output 
params, too?)



Comment at: test/clang-doc/mapper-class-in-class.cpp:1
-// RUN: rm -rf %t
-// RUN: mkdir %t

What's up with all tests being deleted? That is rather disturbing.
I would expect the merge phase to be disable-able, if only just for the testing 
purposes.


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 136793.
juliehockett added a comment.

Updating for parent diff & refactoring reader.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/comment-bc.cpp
  test/clang-doc/mapper-class-in-class.cpp
  test/clang-doc/mapper-class-in-function.cpp
  test/clang-doc/mapper-class.cpp
  test/clang-doc/mapper-comments.cpp
  test/clang-doc/mapper-enum.cpp
  test/clang-doc/mapper-function.cpp
  test/clang-doc/mapper-method.cpp
  test/clang-doc/mapper-namespace.cpp
  test/clang-doc/mapper-struct.cpp
  test/clang-doc/mapper-union.cpp
  test/clang-doc/namespace-bc.cpp
  test/clang-doc/record-bc.cpp

Index: test/clang-doc/record-bc.cpp
===
--- /dev/null
+++ test/clang-doc/record-bc.cpp
@@ -0,0 +1,133 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@E#'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@~E#'
+  // CHECK:  blob data = '~E'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@ProtectedMethod#'
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@ProtectedMethod#'
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@U@A'
+  // CHECK:  blob data = 'A'
+  // CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::X'
+// CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::Y'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@C'
+  // CHECK:  blob data = 'C'
+  // CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'C::i'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@D'
+  // CHECK:  blob data = 'D'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@F'
+  // CHECK:  blob data = 'F'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK:  blob data = 'c:@S@D'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@E@B'
+  // CHECK:  blob data = 'B'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'X'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'Y'
+  // CHECK: 
+// CHECK: 
Index: test/clang-doc/namespace-bc.cpp
===
--- /dev/null
+++ test/clang-doc/namespace-bc.cpp
@@ -0,0 +1,74 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/docs.bc --dump | FileCheck %s
+
+namespace A {
+
+void f();
+void f() {};
+
+} // A
+
+namespace A {
+namespace B {
+
+enum E { X };
+
+E func(int i) { 
+  return X;
+}
+
+}
+}
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK:  blob data = 'A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A@N@B'
+  // CHECK:  blob data = 'B'
+  // CHECK:  blob data = 'c:@N@A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A@F@f#'
+  // CHECK:  blob data = 'f'
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 136660.
juliehockett marked an inline comment as done.
juliehockett added a comment.

Cleaning up some and updating based on changes to the parent diff.


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/mapper-class-in-class.cpp
  test/clang-doc/mapper-class-in-function.cpp
  test/clang-doc/mapper-class.cpp
  test/clang-doc/mapper-comments.cpp
  test/clang-doc/mapper-enum.cpp
  test/clang-doc/mapper-function.cpp
  test/clang-doc/mapper-method.cpp
  test/clang-doc/mapper-namespace.cpp
  test/clang-doc/mapper-struct.cpp
  test/clang-doc/mapper-union.cpp
  test/clang-doc/namespace-bc.cpp
  test/clang-doc/record-bc.cpp

Index: test/clang-doc/record-bc.cpp
===
--- /dev/null
+++ test/clang-doc/record-bc.cpp
@@ -0,0 +1,133 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@E#'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@~E#'
+  // CHECK:  blob data = '~E'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@ProtectedMethod#'
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E@F@ProtectedMethod#'
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@U@A'
+  // CHECK:  blob data = 'A'
+  // CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::X'
+// CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::Y'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@C'
+  // CHECK:  blob data = 'C'
+  // CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'C::i'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@D'
+  // CHECK:  blob data = 'D'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@S@F'
+  // CHECK:  blob data = 'F'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK:  blob data = 'c:@S@D'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@E@B'
+  // CHECK:  blob data = 'B'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'X'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'Y'
+  // CHECK: 
+// CHECK: 
Index: test/clang-doc/namespace-bc.cpp
===
--- /dev/null
+++ test/clang-doc/namespace-bc.cpp
@@ -0,0 +1,74 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/docs.bc --dump | FileCheck %s
+
+namespace A {
+
+void f();
+void f() {};
+
+} // A
+
+namespace A {
+namespace B {
+
+enum E { X };
+
+E func(int i) { 
+  return X;
+}
+
+}
+}
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK:  blob data = 'A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A@N@B'
+  // CHECK:  blob data = 'B'
+  // CHECK:  blob data = 'c:@N@A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'c:@N@A@F@f#'
+  // CHECK:  blob data = 'f'
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'void'

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please run Clang-format and Clang-tidy modernize.




Comment at: clang-doc/BitcodeReader.h:36
+ public:
+  ClangDocBitcodeReader() {}
+  using RecordData = SmallVector;

Please use = default;


https://reviews.llvm.org/D43341



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

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 135520.
juliehockett added a comment.

Updating for parent diff changes


https://reviews.llvm.org/D43341

Files:
  clang-doc/BitcodeReader.cpp
  clang-doc/BitcodeReader.h
  clang-doc/BitcodeWriter.cpp
  clang-doc/BitcodeWriter.h
  clang-doc/CMakeLists.txt
  clang-doc/Reducer.cpp
  clang-doc/Reducer.h
  clang-doc/Representation.cpp
  clang-doc/Representation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/mapper-class.cpp
  test/clang-doc/mapper-enum.cpp
  test/clang-doc/mapper-function.cpp
  test/clang-doc/mapper-method.cpp
  test/clang-doc/mapper-namespace.cpp
  test/clang-doc/mapper-struct.cpp
  test/clang-doc/mapper-union.cpp
  test/clang-doc/namespace.cpp
  test/clang-doc/record.cpp

Index: test/clang-doc/record.cpp
===
--- /dev/null
+++ test/clang-doc/record.cpp
@@ -0,0 +1,107 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+  E() {}
+  ~E() {}
+
+protected:
+  void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = '~E'
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'A'
+  // CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::X'
+// CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::Y'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'C'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'C::i'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'D'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'F'
+  // CHECK: 
+  // CHECK: 
+  // CHECK:  blob data = 'c:@S@E'
+  // CHECK:  blob data = 'c:@S@D'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'B'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'X'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'Y'
+  // CHECK: 
+// CHECK: 
Index: test/clang-doc/namespace.cpp
===
--- /dev/null
+++ test/clang-doc/namespace.cpp
@@ -0,0 +1,67 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
+// RUN: llvm-bcanalyzer %t/docs/bc/docs.bc --dump | FileCheck %s
+
+namespace A {
+
+void f();
+void f() {};
+
+} // A
+
+namespace A {
+namespace B {
+
+enum E { X };
+
+E func(int i) { 
+  return X;
+}
+
+}
+}
+
+// CHECK: 
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'B'
+  // CHECK:  blob data = 'c:@N@A'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'f'
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'void'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'func'
+  // CHECK:  blob data = 'c:@N@A@N@B'
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'enum A::B::E'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'i'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'c:@N@A@N@B'
+  // CHECK:  blob data = 'c:@N@A'
+  // CHECK: 
+  // CHECK: 
+// CHECK:  blob data = 'A::B::X'
+  // CHECK: 
+// CHECK: 
Index: test/clang-doc/mapper-union.cpp
===
--- test/clang-doc/mapper-union.cpp
+++ /dev/null
@@ -1,29 +0,0 @@
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "" > %t/compile_flags.txt
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -output=%t/docs
-// RUN: llvm-bcanalyzer %t/docs/c:@u...@d.bc --dump | FileCheck %s
-
-union D { int X; int Y; };
-// CHECK: 
-// CHECK: 
-  // CHECK: 
-// CHECK: 
-// CHECK: 
-  // CHECK:  blob data = 'D'
-  // 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 134546.
juliehockett edited the summary of this revision.
juliehockett added a comment.

Moving the entire implementation of the bitcode reader to this patch (from the 
mapper patch, here ) and cleaning up 
implementation


https://reviews.llvm.org/D43341

Files:
  clang-doc/CMakeLists.txt
  clang-doc/ClangDocBinary.cpp
  clang-doc/ClangDocBinary.h
  clang-doc/ClangDocReducer.cpp
  clang-doc/ClangDocReducer.h
  clang-doc/ClangDocRepresentation.cpp
  clang-doc/ClangDocRepresentation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/mapper-class.cpp
  test/clang-doc/mapper-enum.cpp
  test/clang-doc/mapper-namespace.cpp
  test/clang-doc/mapper-record.cpp
  test/clang-doc/mapper-struct.cpp
  test/clang-doc/mapper-union.cpp

Index: test/clang-doc/mapper-union.cpp
===
--- test/clang-doc/mapper-union.cpp
+++ /dev/null
@@ -1,26 +0,0 @@
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "" > %t/compile_flags.txt
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -docs=%t/docs
-// RUN: llvm-bcanalyzer %t/docs/A.bc --dump | FileCheck %s
-
-union A { int X; int Y; };
-// CHECK: 
-// CHECK: 
-  // CHECK:  blob data = 'A'
-  // CHECK:  blob data = 'A'
-  // CHECK: 
-  // CHECK: 
-// CHECK: 
-// CHECK:  blob data = 'int'
-// CHECK:  blob data = 'A::X'
-// CHECK: 
-  // CHECK: 
-  // CHECK: 
-// CHECK: 
-// CHECK:  blob data = 'int'
-// CHECK:  blob data = 'A::Y'
-// CHECK: 
-  // CHECK: 
-// CHECK: 
Index: test/clang-doc/mapper-struct.cpp
===
--- test/clang-doc/mapper-struct.cpp
+++ /dev/null
@@ -1,19 +0,0 @@
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "" > %t/compile_flags.txt
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -docs=%t/docs
-// RUN: llvm-bcanalyzer %t/docs/C.bc --dump | FileCheck %s
-
-struct C { int i; };
-// CHECK: 
-// CHECK: 
-  // CHECK:  blob data = 'C'
-  // CHECK:  blob data = 'C'
-  // CHECK: 
-// CHECK: 
-// CHECK:  blob data = 'int'
-// CHECK:  blob data = 'C::i'
-// CHECK: 
-  // CHECK: 
-// CHECK: 
Index: test/clang-doc/mapper-record.cpp
===
--- /dev/null
+++ test/clang-doc/mapper-record.cpp
@@ -0,0 +1,155 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "" > %t/compile_flags.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp -docs=%t/docs
+// RUN llvm-bcanalyzer %t/docs/docs.bc --dump | FileCheck %s
+
+union A { int X; int Y; };
+
+enum B { X, Y };
+
+struct C { int i; };
+
+class D {};
+
+class E {
+public:
+	E() {}
+	~E() {}
+
+protected:
+	void ProtectedMethod();
+};
+
+void E::ProtectedMethod() {}
+
+class F : virtual private D, public E {};
+
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E::E'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = '_ZN1EC1Ev'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'void'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E::~E'
+  // CHECK:  blob data = '~E'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = '_ZN1ED1Ev'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'void'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E::ProtectedMethod'
+  // CHECK:  blob data = 'ProtectedMethod'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = '_ZN1E15ProtectedMethodEv'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'void'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'A'
+  // CHECK:  blob data = 'A'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::X'
+// CHECK: 
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'A::Y'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'C'
+  // CHECK:  blob data = 'C'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'int'
+// CHECK:  blob data = 'C::i'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'D'
+  // CHECK:  blob data = 'D'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'F'
+  // CHECK:  blob data = 'F'
+  // CHECK: 
+  // CHECK:  blob data = 'class E'
+  // CHECK:  blob data = 'class D'
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'B'
+  // CHECK:  blob data = 'B'
+  // CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'X'
+// 

[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added reviewers: klimek, sammccall, jakehehrlich.
juliehockett added a project: clang-tools-extra.
Herald added a subscriber: mgorny.
juliehockett added a dependency: D41102: Setup clang-doc frontend framework.

Implements a simple, in-memory reducer for the mapped output of the initial 
tool. This creates a collection object for storing the deduplicated infos on 
each declaration, and populates that from the mapper output. The collection 
object is serialized to LLVM bitstream. On reading each serialized output, it 
checks to see if a merge is necessary and if so, merges the new info with the 
existing info (prefering the existing one if conflicts exist).

For a more detailed overview of the tool, see the design document on the 
mailing list: RFC: clang-doc proposal 



https://reviews.llvm.org/D43341

Files:
  clang-doc/CMakeLists.txt
  clang-doc/ClangDocBinary.cpp
  clang-doc/ClangDocBinary.h
  clang-doc/ClangDocMapper.cpp
  clang-doc/ClangDocMapper.h
  clang-doc/ClangDocReducer.cpp
  clang-doc/ClangDocReducer.h
  clang-doc/ClangDocRepresentation.cpp
  clang-doc/ClangDocRepresentation.h
  clang-doc/tool/ClangDocMain.cpp
  test/clang-doc/mapper-namespace.cpp
  test/clang-doc/mapper-type.cpp

Index: test/clang-doc/mapper-type.cpp
===
--- test/clang-doc/mapper-type.cpp
+++ test/clang-doc/mapper-type.cpp
@@ -2,136 +2,154 @@
 // RUN: mkdir %t
 // RUN: echo "" > %t/compile_flags.txt
 // RUN: cp "%s" "%t/test.cpp"
-// RUN: clang-doc  --dump --omit-filenames -doxygen -p %t %t/test.cpp | FileCheck %s
+// RUN: clang-doc --dump --omit-filenames -doxygen -p %t %t/test.cpp > %t/info.bc
+// RUN llvm-bcanalyzer %t/info.bc --dump | FileCheck %s
 
 union A { int X; int Y; };
-// CHECK: ---
-// CHECK: KEY: A
-// CHECK: FullyQualifiedName: A
-// CHECK: Name: A
-// CHECK: TagType: 2
-// CHECK: ID: Member
-// CHECK: Type: int
-// CHECK: Name: A::X
-// CHECK: Access: 3
-// CHECK: ID: Member
-// CHECK: Type: int
-// CHECK: Name: A::Y
-// CHECK: Access: 3
-// CHECK: ---
-// CHECK: KEY: A::A
-// CHECK: FullyQualifiedName: A::A
-// CHECK: Name: A
-// CHECK: Namespace: A
 
 enum B { X, Y };
-// CHECK: ---
-// CHECK: KEY: B
-// CHECK: FullyQualifiedName: B
-// CHECK: Name: B
-// CHECK: ID: Member
-// CHECK: Type: X
-// CHECK: Access: 3
-// CHECK: ID: Member
-// CHECK: Type: Y
-// CHECK: Access: 3
 
 struct C { int i; };
-// CHECK: ---
-// CHECK: KEY: C
-// CHECK: FullyQualifiedName: C
-// CHECK: Name: C
-// CHECK: ID: Member
-// CHECK: Type: int
-// CHECK: Name: C::i
-// CHECK: Access: 3
-// CHECK: ---
-// CHECK: KEY: C::C
-// CHECK: FullyQualifiedName: C::C
-// CHECK: Name: C
-// CHECK: Namespace: C
 
 class D {};
-// CHECK: ---
-// CHECK: KEY: D
-// CHECK: FullyQualifiedName: D
-// CHECK: Name: D
-// CHECK: TagType: 3
-// CHECK: ---
-// CHECK: KEY: D::D
-// CHECK: FullyQualifiedName: D::D
-// CHECK: Name: D
-// CHECK: Namespace: D
 
 class E {
-// CHECK: ---
-// CHECK: KEY: E
-// CHECK: FullyQualifiedName: E
-// CHECK: Name: E
-// CHECK: TagType: 3
-// CHECK: ---
-// CHECK: KEY: E::E
-// CHECK: FullyQualifiedName: E::E
-// CHECK: Name: E
-// CHECK: Namespace: E
-
 public:
 	E() {}
-// CHECK: ---
-// CHECK: KEY: _ZN1EC1Ev
-// CHECK: FullyQualifiedName: E::E
-// CHECK: Name: E
-// CHECK: Namespace: E
-// CHECK: MangledName: _ZN1EC1Ev
-// CHECK: Parent: E
-// CHECK: ID: Return
-// CHECK: Type: void
-// CHECK: Access: 3
-
-	 ~E() {}
-// CHECK: ---
-// CHECK: KEY: _ZN1ED1Ev
-// CHECK: FullyQualifiedName: E::~E
-// CHECK: Name: ~E
-// CHECK: Namespace: E
-// CHECK: MangledName: _ZN1ED1Ev
-// CHECK: Parent: E
-// CHECK: ID: Return
-// CHECK: Type: void
-// CHECK: Access: 3
+	~E() {}
 
 protected:
 	void ProtectedMethod();
-// CHECK:  ---
-// CHECK: KEY: _ZN1E15ProtectedMethodEv
-// CHECK: FullyQualifiedName: _ZN1E15ProtectedMethodEv
-// CHECK: Name: ProtectedMethod
-// CHECK: Namespace: E
 };
 
 void E::ProtectedMethod() {}
-// CHECK: ---
-// CHECK: KEY: _ZN1E15ProtectedMethodEv
-// CHECK: FullyQualifiedName: E::ProtectedMethod
-// CHECK: Name: ProtectedMethod
-// CHECK: Namespace: E
-// CHECK: MangledName: _ZN1E15ProtectedMethodEv
-// CHECK: Parent: E
-// CHECK: ID: Return
-// CHECK: Type: void
-// CHECK: Access: 3
-// CHECK: Access: 1
 
 class F : virtual private D, public E {};
-// CHECK: ---
-// CHECK: KEY: F
-// CHECK: FullyQualifiedName: F
-// CHECK: Name: F
-// CHECK: TagType: 3
-// CHECK: Parent: class E
-// CHECK: VParent: class D
-// CHECK: ---
-// CHECK: KEY: F::F
-// CHECK: FullyQualifiedName: F::F
-// CHECK: Name: F
-// CHECK: Namespace: F
+
+// CHECK: 
+// CHECK: 
+  // CHECK:  blob data = 'E::E'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = 'E'
+  // CHECK:  blob data = '_ZN1EC1Ev'
+  // CHECK:  blob data = 'E'
+  // CHECK: 
+// CHECK: 
+// CHECK:  blob data = 'void'
+// CHECK: 
+  // CHECK: 
+// CHECK: 
+// CHECK: 
+  //