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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Lex/DependencyDirectivesSourceMinimizer.h:36
+  pp_import,
+  pp_at_import,
+  pp_pragma_import,
----------------
Bigcheese wrote:
> Is `@import` actually a preprocessor directive? For C++20 modules all the modules bits end up being declarations.
No, `@import` is actually a declaration. I've updated the comments.


================
Comment at: lib/Frontend/FrontendActions.cpp:923-924
+                                           Toks)) {
+    CI.getDiagnostics().Report(
+        diag::err_minimize_source_to_dependency_directives_failed);
+    return;
----------------
Bigcheese wrote:
> Can we give better error messages here?  Should `minimizeSourceToDependencyDirectives` take a `DiagnosticEngine`?
Sure, I added support for diagnostics!


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


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

https://reviews.llvm.org/D55463