rsmith added a comment.

Patch generally looks good; just a minor concern about the output format.



================
Comment at: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp:487-490
+            OS << " - evaluated by -frewrite-includes */" << MainEOL;
+            OS << (elif ? "#elif" : "#if");
+            OS << " (" << (isTrue ? "1" : "0")
+               << ") /* evaluated by -frewrite-includes */";
----------------
Adding an extra line here is going to mess up presumed line numbering. Also, we 
don't need parentheses around the constant `0` or `1`. Perhaps instead we could 
prepend a `#if 0 /*` or `#if 1 /*` to the directive:

```
#if FOO(x) == \
  BAZ
#ifndef BAR
```

->

```
#if 1 /*#if FOO(x) == \
  BAZ*/
#if 0 /*#ifndef BAR*/
```

I don't think we really need the "evaluated by -frewrite-includes" part, but I 
have no objection to including it. The best place is probably between the `/*` 
and the `#`:

```
#if 1 /*evaluated by -frewrite-includes: #if FOO(x) == \
  BAZ*/
#if 0 /*evaluated by -frewrite-includes: #ifndef BAR*/
```



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
  • [PATCH] D63508: m... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to