Details
Diff Detail
Event Timeline
Can you add a test? Tests for this kind of behavior of the clang-format command line go in test/Format/.
| tools/clang-format/ClangFormat.cpp | ||
|---|---|---|
| 375 | As clang-format often returns the result on stdout, maybe it'd be better to use errs() here? | |
| tools/clang-format/ClangFormat.cpp | ||
|---|---|---|
| 375 | Right, otherwise, this: | |
Generally upload diffs with full context to phabricator. That makes reviewing much easier.
| test/Format/verbose.cpp | ||
|---|---|---|
| 3 | This seems like a pretty minimal test to me. Consider adding the filename (with a regex like "Formatting{{.*}}verbose.cpp"). There might be reasons not to do that though. | |
| tools/clang-format/ClangFormat.cpp | ||
| 377 | I think we should restructure the code to not have to duplicate what you are adding here. I think fundamentally, we should be able to change this to: if (FileNames.empty()) {
Error = clang::format::format("-");
return Error ? 1 : 0;
}
if (FileNames.size() == 1 && (!Offsets.empty() || !Lengths.empty() || !LineRanges.empty())) {
errs() << "error: -offset .... "
return 1;
}
for (const auto& FileName : FileNames) {
if (Verbose.getNumOccurences() != 0)
errs() << "Formatting " << Filename << "\n";
Error |= clang::format::format(FileName);
}
return Error ? 1 : 0; | |
| 388 | Feel free to turn this into a range-based for loop while you are here. :) | |
| 390 | Not sure that just checking the number of occurrences is correct. What happens if I write -verbose=false on the command line? | |
| 391 | The indentation is off. | |
| tools/clang-format/ClangFormat.cpp | ||
|---|---|---|
| 377 | This isn't correct. bin/clang-format -lines=1:1 foo-1.cpp foo-2.cpp we won't enter into the "error: -offset" error | |
| test/Format/verbose.cpp | ||
|---|---|---|
| 3 | I changed the structure and added more checks to it. | |
| tools/clang-format/ClangFormat.cpp | ||
|---|---|---|
| 377 | Right. In my code I had a bug s/== 1/ != 1/. But I don't think you can drop that part entirely. As it is now, you cannot specify any -lines/offsets/lenghts even for a single file? | |
Fixed, thanks for spotting the mistake.
Looks like we could improve the testsuite as my mistake hasn't been catched.
Yeah, improving the testsuite generally seems like a good idea. But unrelated to this patch. This looks good now.
This seems like a pretty minimal test to me. Consider adding the filename (with a regex like "Formatting{{.*}}verbose.cpp"). There might be reasons not to do that though.