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