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 https://revi
bruno added a comment.
The test is failing on windows:
http://lab.llvm.org:8011/builders/clang-x86-windows-msvc2015/builds/141
I reverted the patch for now in r285416, can you take a look?
Repository:
rL LLVM
https://reviews.llvm.org/D25153
___
This revision was automatically updated to reflect the committed changes.
Closed by commit rL285411: [Preprocessor] Support for '-dI' flag (authored by
bruno).
Changed prior to commit:
https://reviews.llvm.org/D25153?vs=75351&id=76210#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25153
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
htt
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, i.e.
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 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
http://lists.llvm.org/cg
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
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Thanks for your patience, this looks great to me. Do you need someone to commit
this for you?
https://reviews.llvm.org/D25153
___
cfe-commits ma
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.
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 p
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
include/clang/Frontend/Preprocesso
vsk added inline comments.
Comment at: test/Preprocessor/dump_include.c:3
+// RUN: %clang_cc1 -w -E -dI -isystem %S %s -o - | grep '^#include *"dump_'
+// RUN: %clang_cc1 -w -E -dI -isystem %S %s -o - | grep '^#include_next
*"dump_'
+// RUN: %clang_cc1 -w -E -dI -isystem %S -ima
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
http://lists.llvm.org/cgi-bin/m
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
___
cfe-com
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
___
cfe-co
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
include/clang/Frontend/PreprocessorOut
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 reso
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 blo
majnemer added a comment.
The change from an algorithmic POV looks good but I can't speak to whether or
not the approach is sound, I'll leave that to @rsmith.
> majnemer wrote in PrintPreprocessedOutput.cpp:404
> !SearchPath.empty()
This review comment was never addressed.
https://reviews.ll
elsteveogrande added a comment.
@rsmith @majnemer Ready to review once more.
(Pardon the nag but I wanted to ensure you knew. If there's another round of
fixes I'll change to "changes planned", fix, and then move back to "needs
review" when ready again. So you'll know if it's in a ready state
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
string
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
include/clang/Frontend/Preproce
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("\\")) {
> Buffer.push_back("
majnemer added inline comments.
> PrintPreprocessedOutput.cpp:331-349
> + const size_t N = Path.size();
> + size_t I = 0;
> + while (I < N) {
> +if (Path[I] == '\\' || Path[I] == '\"') {
> + // Have to escape backslashes or double-quotes.
> + // Send out backslash to escape the n
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
lib/Fronten
elsteveogrande added a comment.
Thanks -- will do one more pass
> majnemer wrote in PrintPreprocessedOutput.cpp:409-411
> Why not have sanitizePath return a `std::vector`?
Hmm, good idea. Better than caller-passed vectors, and it be whatever size is
needed (in the called method I'd `CharVect
majnemer added inline comments.
> PrintPreprocessedOutput.cpp:101
>PrintPPOutputPPCallbacks(Preprocessor &pp, raw_ostream &os, bool
> lineMarkers,
> - bool defines, bool UseLineDirectives)
> + bool defines, bool dumpIncludeDirectives,
> +
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`, `-imacro`
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 `PP.getSpelling
majnemer added inline comments.
> majnemer wrote in PrintPreprocessedOutput.cpp:321-325
> Variables should start with uppercase characters.
Please uppercase all your other variables too.
> PrintPreprocessedOutput.cpp:344
> + */
> +bool tryGetTokenText(StringRef *text, const Token &tok) {
> + i
elsteveogrande added a comment.
Thanks again @rsmith! Updates will be coming; I have some other fixes as well.
> rsmith wrote in PrintPreprocessedOutput.cpp:329
> Is this really necessary? It'll be very ugly on Windows.
Yeah, there will be tons of double-backslashes because of this. I think
rsmith added inline comments.
> PrintPreprocessedOutput.cpp:328
> + case '"': // paths are enclosed in quotes; escape them
> + case '*': // don't allow a "*/" sequence to accidentally open the
> comment
> + case '\\': // escape the escape character itself.
Putting a \ before a
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, et
majnemer added inline comments.
> PrintPreprocessedOutput.cpp:321
> + */
> +std::string sanitizePath(const StringRef& path) {
> + std::string str(path.size() * 2, '\0');
Don't pass `StringRef` by const reference, just pass it by value.
> PrintPreprocessedOutput.cpp:321-325
> +std::string sanit
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:
include/clang/Driver/Options.td
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 mor
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.)
rsmith added inline comments.
> PrintPreprocessedOutput.cpp:320-321
> + * if we're emitting a '#'-style directive (escape both CR and LF). Paths
> are quoted in some
> + * instances, so escape quotes. Escaping is with a backslash (and
> backslashes themselves have
> + * to be escaped); for CR
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
http://lists.llvm.org/cgi-bin/
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
include
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 :)
> PrintPrep
42 matches
Mail list logo
