[PATCH] D145244: [clang-format] Add ability to trace tokens.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:289
                                    Current.closesBlockOrBlockTypeList(Style))) {
+    DEBUG_FORMAT_TRACE_TOKEN(Current, "!canBreak");
     return false;
----------------
annotating all exit paths from this function and `mustBreak` seems much more verbose and fragile than wrapping the function (making this version private) and adding the annotations in the wrapper.


================
Comment at: clang/lib/Format/FormatDebug.h:22
+
+#ifndef NDEBUG
+
----------------
it looks like you're not providing a dummy definition in NDEBUG mode - does this still build in that config?


================
Comment at: clang/lib/Format/FormatDebug.h:28
+
+#define DEBUG_FORMAT_TRACE_TOKEN(Tok, Debug)                                   \
+  if (internal::DebugTraceToken().match((Tok).TokenText)) {                    \
----------------
Comment with an example?
(In particular, unclear what "Debug" means)


================
Comment at: clang/lib/Format/FormatDebug.h:32
+                 << (Tok).TokenText << ": " << Debug << "\n";                  \
+  } else {                                                                     \
+  }
----------------
why empty else?


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3258
       }
+      DEBUG_FORMAT_TRACE_TOKEN(*Current, (Current->MustBreakBefore ? "" : "!")
+                                             << "MustBreakBefore");
----------------
move this out of the if() and remove the one on the other branch?


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3258
       }
+      DEBUG_FORMAT_TRACE_TOKEN(*Current, (Current->MustBreakBefore ? "" : "!")
+                                             << "MustBreakBefore");
----------------
sammccall wrote:
> move this out of the if() and remove the one on the other branch?
this meaning of `<<` in `(...) << "MustBreakBefore` is confusing.

consider `<< (...) << "MustBreakBefore"` or `dbgs() << (...) << "MustBreakBefore"` or a twine or formatv or really anything that isn't a shift :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145244