amaiorano added a comment.
In https://reviews.llvm.org/D28081#633521, @ioeric wrote:
> Some nits. Some is almost good :)
>
> BTW, do you have clang-tools-extra in your source tree? There are also some
> references in the subtree to the changed interface. It would be nice if you
> could also fix them in a separate patch and commit these two patches together
> (I mean, within a short period of time) so that you wouldn't break build bots.
>
> References should be found in these files:
>
> extra/change-namespace/ChangeNamespace.cpp
> extra/clang-move/ClangMove.cpp
> extra/include-fixer/tool/ClangIncludeFixer.cpp
> extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
> extra/clang-tidy/ClangTidy.cpp
>
>
> Thanks!
I'll grab clang-tools-extras and make the second patch as you suggest. Btw, can
you explain how I would avoid breaking build bots? I assume you mean that
clang-tools-extras gets built separately against some version of clang, which
gets auto-updated. When would I know the right time to push the second patch
through?
Also, I assume I'd have to get this second patch approved before ever pushing
the first, right?
================
Comment at: lib/Format/Format.cpp:424
+llvm::Error make_string_error(const llvm::Twine &Message) {
+ return llvm::make_error<llvm::StringError>(Message,
----------------
ioeric wrote:
> Maybe make this `inline`?
Yes.
================
Comment at: lib/Format/Format.cpp:1901
+ // FIXME: If FallbackStyle is explicitly "none", this effectively disables
+ // replacements. Fix this by setting a separate FormatStyle variable and
+ // returning it when we mean to return the fallback style explicitly.
----------------
ioeric wrote:
> I'd state the problem with a specific solution only if I am sure it is the
> best solution.
Good point, will remove the solution.
================
Comment at: lib/Tooling/Refactoring.cpp:86
- format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+ llvm::Expected<format::FormatStyle> FormatStyleOrError =
+ format::getStyle(Style, FilePath, "LLVM");
----------------
ioeric wrote:
> There is a `NewReplacements` below which is also `llvm::Expected<T>`.
> `FormatStyleOrError` is a fine name, but to be we have been naming
> `Expected` types without `OrError` postfix, so I'd go without `OrError`
> postfix for consistency append `OrError` postfix to other expected variables.
> Personally, I find expected variables without `OrError` postfix easier to
> understand, especially in error checking. For example, `if
> (!FormatStyleOrError)` is a bit awkward to read while `if (!FormatStyle)` is
> more straight-forward IMO.
>
> Same for other changes.
Agreed. For consistency, would you prefer I also use 'auto' for the return type
rather than llvm::Expected<format::FormatStyle> as is done for NewReplacements?
================
Comment at: unittests/Format/FormatTest.cpp:10972
+ auto ErrorMsg4 = llvm::toString(Style4.takeError());
+ ASSERT_GT(ErrorMsg4.length(), 0);
+
----------------
ioeric wrote:
> This is a bit strange... Just `llvm::consumeError(Style.takeError())` if you
> don't bother to check the error message, e.g. with
> `llvm::StringRef::starswith` or RE match.
Yeah, I'll go with the consumeError.
================
Comment at: unittests/Format/FormatTestObjC.cpp:72
TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
- Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+ Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
"- (id)init;");
----------------
ioeric wrote:
> amaiorano wrote:
> > In these tests, I'm assuming getStyle returns a valid FormatSyle. I could
> > add the same types of validation as in the FormatStyle.GetStyleOfFile tests.
> Please add proper checking as above for returned values.
Hmm, so I could replace the Style member of the fixture class with
Expected<FormatStyle>, and then change all "Style." with "Style->" in the rest
of the test file, or only in this specific test, I could store the result in a
local Expected<FormatStyle>, check that it's valid, and then assign to Style.
The latter is simpler; only question I have is how to name the local variable -
can I go with StyleOrError? Style2?
https://reviews.llvm.org/D28081
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
