[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese requested changes to this revision.
Bigcheese added a comment.
This revision now requires changes to proceed.

I have a bit more review to do, but this is what I've found so far.  The naming comments are just suggestions, but the digit separators' are actually an issue.



================
Comment at: include/clang/Lex/DependencyDirectivesSourceMinimizer.h:25
+namespace clang {
+namespace minimize_source_to_dependency_directives {
+
----------------
This is a really long namespace name, not sure what else to call it though.


================
Comment at: include/clang/Lex/DependencyDirectivesSourceMinimizer.h:36
+  pp_import,
+  pp_at_import,
+  pp_pragma_import,
----------------
Is `@import` actually a preprocessor directive? For C++20 modules all the modules bits end up being declarations.


================
Comment at: lib/Frontend/FrontendActions.cpp:923-924
+                                           Toks)) {
+    CI.getDiagnostics().Report(
+        diag::err_minimize_source_to_dependency_directives_failed);
+    return;
----------------
Can we give better error messages here?  Should `minimizeSourceToDependencyDirectives` take a `DiagnosticEngine`?


================
Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:28
+
+struct Lexer {
+  SmallVectorImpl<char> &Out;
----------------
I'm not sure `Lexer` is the best name for this class.  It's really a combined lexer and minimizer and I was a bit confused by some things until I realized that.  I think it would make more sense to name this `Minimizer` and the associated `lex` as `minimize`.


================
Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:203
+
+static const char *reverseOverSpaces(const char *First, const char *Last) {
+  while (First != Last && isHorizontalWhitespace(Last[-1]))
----------------
`assert(First <= Last)` to match the other checks?


================
Comment at: lib/Lex/DependencyDirectivesSourceMinimizer.cpp:241
+      // Iterate over strings correctly to avoid comments and newlines.
+      if (*First == '\"' || *First == '\'') {
+        if (isRawStringLiteral(Start, First))
----------------
I don't think this handles digit separators correctly.
```
#include <bob>
int a = 0'1;
#include <foo>
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D55463