[PATCH] D63508: make -frewrite-includes also rewrite conditions in #if/#elif

[Luboš Luňák via Phabricator via cfe-commits]

llunak added inline comments.


================
Comment at: clang/test/Frontend/rewrite-includes-conditions.c:17
+line4
+#elif value2 < value2
+line5
----------------
rsmith wrote:
> Did you mean for this to be `value1 < value2` rather than `value2 < value2`?
Yes, not that it'd matter in practice. I'll fix that in the next iteration of the patch.



================
Comment at: clang/test/Frontend/rewrite-includes-conditions.c:54-67
+// CHECK: #if 0 /* disabled by -frewrite-includes */
+// CHECK-NEXT: #if value1 == value2
+// CHECK-NEXT: #endif
+// CHECK-NEXT: #endif /* disabled by -frewrite-includes */
+// CHECK-NEXT: #if 0 /* evaluated by -frewrite-includes */
+// CHECK-NEXT: # 14 "{{.*}}rewrite-includes-conditions.c"
+
----------------
rsmith wrote:
> I find this output pretty hard to read -- it's hard to tell what's an original `#if` / `#endif` that's been effectively commented out, and what's an added `#if` / `#endif` that's doing the commenting.
> 
> What do you think about modifying the original line so it's no longer recognized as a preprocessing directive? For instance, instead of:
> 
> ```
> #if 0 /* disabled by -frewrite-includes */
> #if value1 == value2
> #endif
> #endif /* disabled by -frewrite-includes */
> #if 0 /* evaluated by -frewrite-includes */
> # 14 "{{.*}}rewrite-includes-conditions.c"
> line3
> #if 0 /* disabled by -frewrite-includes */
> #if 0
> #elif value1 > value2
> #endif
> #endif /* disabled by -frewrite-includes */
> #elif 0 /* evaluated by -frewrite-includes */
> # 16 "{{.*}}rewrite-includes-conditions.c"
> line4
> #if 0 /* disabled by -frewrite-includes */
> #if 0
> #elif value1 < value2
> #endif
> #endif /* disabled by -frewrite-includes */
> #elif 1 /* evaluated by -frewrite-includes */
> # 18 "{{.*}}rewrite-includes-conditions.c"
> line5
> [...]
> ```
> 
> you might produce
> 
> ```
> #if 0 /* rewritten by -frewrite-includes */
> !#if value1 == value2
> #endif
> #if 0 /* evaluated by -frewrite-includes */
> # 14 "{{.*}}rewrite-includes-conditions.c"
> line3
> #if 0 /* rewritten by -frewrite-includes */
> !#elif value1 > value2
> #endif
> #elif 0 /* evaluated by -frewrite-includes */
> # 16 "{{.*}}rewrite-includes-conditions.c"
> line4
> #if 0 /* rewritten by -frewrite-includes */
> !#elif value1 < value2
> #endif
> #elif 1 /* evaluated by -frewrite-includes */
> # 18 "{{.*}}rewrite-includes-conditions.c"
> line5
> [...]
> ```
> 
> (or whatever transformation you like that prevents recognition of a `#if`/`#elif` within the `#if 0` block).
> 
> Also, maybe we could move the quoted directives inside their respective `#if` / `#elif` block, and only comment them out if the block was entered:
> 
> ```
> #if 0 /* evaluated by -frewrite-includes */
> was: #if value1 == value2
> # 14 "{{.*}}rewrite-includes-conditions.c"
> line3
> #elif 0 /* evaluated by -frewrite-includes */
> was: #elif value1 > value2
> # 16 "{{.*}}rewrite-includes-conditions.c"
> line4
> #elif 1 /* evaluated by -frewrite-includes */
> #if 0
> was: #elif value1 < value2
> #endif
> # 18 "{{.*}}rewrite-includes-conditions.c"
> line5
> [...]
> ```
I think that's a matter of taste/opinion. I find your proposal visually harder to read, because although it saves one line, it also is incorrect syntax, it visually breaks if-endif nesting and is inconsistent with how -frewrite-includes does other rewriting. Moreover I think this will be so rarely examined by a human that it doesn't really matter. So unless you insist I'd prefer to keep it as it is.

BTW, the other related review I linked above, is it enough to mention it here, or should I do something more about it? I'm not sure how to pick reviewers if there's not an obvious one.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63508