[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file
David Blaikie via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 4 08:54:07 PDT 2017
dblaikie added a comment.
Couple of drive by comments - no doubt Richard will have a more in depth review, but figured this might save a round or two.
================
Comment at: lib/Frontend/CompilerInvocation.cpp:1561-1565
+ if (File.empty())
+ {
+ Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+ return;
+ }
----------------
Run the whole change through clang-format (there are tools to help do this over a patch or git revision range) so it matches LLVM coding conventions
================
Comment at: lib/Frontend/CompilerInvocation.cpp:1576
+ StringRef Str = Buf.get()->getBuffer();
+ for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+ {
----------------
Prefer copy init over direct init:
for (size_t B = 0, E = 0;
& probably != rather than < is more canonical/common in the LLVM codebase.
================
Comment at: lib/Frontend/CompilerInvocation.cpp:1586
+ // [B, E) is our line. Compare and skip the prefix, if any.
+ StringRef Line(Str.data() + B, E - B);
+ if (!Prefix.empty())
----------------
There's probably a StringRef-y way to do this rather than going back through raw pointers? (StringRef::substr or the like)
https://reviews.llvm.org/D37299
More information about the cfe-commits
mailing list