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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
