elsteveogrande added a comment.
Looks like I have the "accept" ability in this repo, wasn't sure since I just
registered for this recently :)
I think since the tests look good, it's a nice and elegant 1-line change, per
the reviewer's (@rsmith 's) comments, this should be ok to go.
This would
elsteveogrande marked an inline comment as done.
elsteveogrande added inline comments.
> majnemer wrote in PrintPreprocessedOutput.cpp:331-349
> I think this loop would be easier to understand like so:
>
> while (!Path.empty()) {
> if (Path.consume_front("\\")) {
>
elsteveogrande updated this revision to Diff 73720.
elsteveogrande marked an inline comment as done.
elsteveogrande added a comment.
Using `consume_front(sequence)`, cleaner escaping code.
https://reviews.llvm.org/D25153
Files:
include/clang/Driver/Options.td
elsteveogrande updated this revision to Diff 73625.
elsteveogrande added a comment.
Addressed most recent comments, hopefully squashed the last few things.
https://reviews.llvm.org/D25153
Files:
include/clang/Driver/Options.td
include/clang/Frontend/PreprocessorOutputOptions.h
elsteveogrande updated this revision to Diff 73518.
elsteveogrande added a comment.
- Fix a few more style nits according to LLVM style guide.
- Further fixed w/ `clang-format -style=LLVM`, kept (most of) recommended
changes.
- Added more unit tests (more include cases: `#include_next`,
elsteveogrande added a comment.
Thanks again @majnemer! Other vars are fixed, will send an update shortly.
> majnemer wrote in PrintPreprocessedOutput.cpp:344
> Could this just return a StringRef? You could use an empty StringRef on
> failure.
I'll get rid of this in favor of
elsteveogrande added a comment.
Hi @rsmith -- now this simply reports the `#include` line (or whatever token)
without fiddly path escaping. So this is simplified and there's less room for
error and such.
https://reviews.llvm.org/D25153
___
elsteveogrande added inline comments.
> PrintPreprocessedOutput.cpp:329-330
> +static std::string sanitizePath(StringRef Path) {
> + std::string Result;
> + Result.reserve(Path.size() * 2);
> + while (!Path.empty()) {
Note: I'm //pretty// sure this is about as efficient as it gets for
elsteveogrande updated this revision to Diff 73916.
elsteveogrande added a comment.
Dropped escaping function, adds logic to this diff which isn't 100% necessary
at this time. See updated description, under test plan, for details. The
output already does hint at the path which was used to
elsteveogrande updated this revision to Diff 73917.
elsteveogrande updated the summary for this revision.
elsteveogrande added a comment.
update description w/ `arc diff --verbatim`
https://reviews.llvm.org/D25153
Files:
include/clang/Driver/Options.td
elsteveogrande added inline comments.
> PrintPreprocessedOutput.cpp:388-394
> + if (SearchPath.size() > 0) {
> +// Print out info about the search path within this comment. We need
> +// to escape it because we're printing a quoted string within the
> +// comment
elsteveogrande added a comment.
ping :) Wanted to get sign off on this (simplified) diff if possible.
Thanks! Sorry to nag. (This would be one part of the solution for really long
build times for us.)
https://reviews.llvm.org/D25153
___
elsteveogrande accepted this revision.
elsteveogrande added a reviewer: elsteveogrande.
elsteveogrande added a comment.
This revision is now accepted and ready to land.
Thought I'd take a look at the list of diffs and take a stab at a few (a few
easy-looking and short ones :) ) to unblock some
elsteveogrande added a comment.
Ping :) Hoping someone on cfe-commits might be able to take a quick look...
Repository:
rL LLVM
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
elsteveogrande added a comment.
Also the other issues are old comments, now fixed with the latest updates to
this. This does now take the value of the include token: `include` or
`include_once` or `import` etc.; and send that to output.
Thanks very much @rsmith ! (Sorry to have been a pest.)
elsteveogrande added inline comments.
> elsteveogrande wrote in PrintPreprocessedOutput.cpp:320-321
> right you are! I was thinking of `//` when I was doing that escaping logic.
>
> I should either escape `*/`, or even better, just use `//` for comments, is
> that ok? (I feel like `//` is
elsteveogrande added a comment.
Added a couple of notes
> PrintPreprocessedOutput.cpp:241
>}
> -
>return false;
Re-reading this diff, I saw hat my editor trimmed EOL whitespace automatically
and I let these minor changes become part of the patch. Sorry for the noise :)
>
elsteveogrande updated this revision to Diff 73224.
elsteveogrande added a comment.
Updated to actually use the include token's text, not a hardcoded `#include`.
Updated unit test to expect the right word here.
https://reviews.llvm.org/D25153
Files:
include/clang/Driver/Options.td
elsteveogrande updated this revision to Diff 73398.
elsteveogrande added a comment.
Thanks for the feedback @majnemer! Fixed these. I _think_ I used `StringRef`
right; I assume it's like `std::string` in that it manages its own memory (i.e.
I didn't create references to deallocated strings,
elsteveogrande updated this revision to Diff 73394.
elsteveogrande updated the summary for this revision.
elsteveogrande added a comment.
Fixed escaping function; simplified that function as well as the diagnostic
output.
https://reviews.llvm.org/D25153
Files:
elsteveogrande added a comment.
cc a few more devs who have dealt with frontend lately and might be familiar
with this part. :)
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
elsteveogrande added inline comments.
Comment at: cfe/trunk/test/Preprocessor/dump_include.c:2
+// RUN: %clang_cc1 -w -E -dI -isystem %S -imacros %S/dump_include.h %s -o - |
FileCheck %s
+// CHECK: {{^}}#__include_macros "/{{([^/]+/)+}}dump_
+// CHECK: {{^}}#include
elsteveogrande added a comment.
ping - seems I'm unable to commit this myself. Thanks!
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
elsteveogrande updated this revision to Diff 75351.
elsteveogrande added a comment.
Fixed an error. A newline is sometimes not appended prior to this `#include`.
When returning from an included file which doesn't have a trailing newline, the
#include is stuck at the end of some other line,
elsteveogrande added a comment.
Ping, can anyone help with committing this? It's accepted, just needs to be
landed. (I don't have commit access.) Thanks!
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
elsteveogrande updated this revision to Diff 74265.
elsteveogrande added a comment.
Thanks very much @vsk for the feedback! Updated per your recommendations;
`CHECK` looks much better
https://reviews.llvm.org/D25153
Files:
include/clang/Driver/Options.td
elsteveogrande added a comment.
Ping again -- addressed all issues, looking ok now?
Note about this change + review process here. First I know the every 1-2 day
pinging generates noise on the lists, and for that I'm SorryNotSorry :-p
I just want to ensure this is reaching at least the right
elsteveogrande added a comment.
Ping
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
elsteveogrande added a comment.
Thank you! I tried with`arc land` but got a 403...
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
elsteveogrande added a comment.
I'll need help landing this... I also tried `git svn dcommit` but command hung
there forever.
Thanks!
https://reviews.llvm.org/D25153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
30 matches
Mail list logo
