dexonsmith added inline comments.
================
Comment at: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp:175
"#define MACRO con tent ", Out));
- EXPECT_STREQ("#define MACRO con tent\n", Out.data());
+ EXPECT_STREQ("#define MACRO con tent\n", Out.data());
----------------
akyrtzi wrote:
> dexonsmith wrote:
> > Can you clarify why this is changing in this patch? It’s not obvious to me
> > why this would start optimizing internal whitespace of macros.
> >
> > (I also can’t remember if it’s safe/correct. Is there a way to stringify
> > the contents of a macro? If so, this would change the result… if not, then
> > this seems like an improvement, but maybe better for a separate patch?)
> >
> > (If there’s a good reason to change it here, don’t want to hold it up, but
> > it’s not explained in the commit message so it’s not obvious to me.)
> > Can you clarify why this is changing in this patch? It’s not obvious to me
> > why this would start optimizing internal whitespace of macros.
>
> Minimization was doing `printToNewline()` after the macro identifier, which
> just prints verbatim, including any amount of whitespace. OTOH the new way is
> collecting the tokens of the directive, which ignores unnecessary whitespace.
>
> > (I also can’t remember if it’s safe/correct. Is there a way to stringify
> > the contents of a macro? If so, this would change the result…
>
> AFAIK this is correct since the macro body is getting tokenized when the
> preprocessor sees a `#define` and the unnecessary whitespace is ignored.
>
> > if not, then this seems like an improvement, but maybe better for a
> > separate patch?)
>
> To clarify, `minimizeSourceToDependencyDirectives()` is only used for testing
> purposes, once we get the tokens of the directives then we use them directly
> during preprocessing, so there's no place for preserving the unnecessary
> whitespace. This change is just inherent to the difference in implementation
> for minimization vs tokenization.
SGTM! Thanks for explaining. (I’ll let others review in detail!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125487/new/
https://reviews.llvm.org/D125487
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
