[clang-tools-extra] r221272 - [clang-tidy] Don't print a message if there's no error.

[Alexander Kornienko]

The intent of this code is to allow handling of three distinct cases:
  1. a valid ClangTidyOptions instance
  2. an error with a message
  3. no error and no ClangTidyOptions, so the code just goes on looking for
a configuration file without displaying an error

Case 3 could be handled by introducing a separate return-by-reference
boolean flag or something like that, but here I preferred to use a valid
state of the ErrorOr class: HasError + std::error_code containing 0
(success). If this looks confusing, I can add a comment describing this
case.

On Tue, Nov 4, 2014 at 12:24 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Tue, Nov 4, 2014 at 12:23 PM, Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> David Blaikie <dblaikie at gmail.com> writes:
>> >     This is totally wrong. ErrorOr's implicit bool conversion is true
>> iff
>> >     there is an error,
>> >
>> > Doesn't look like it:
>> >
>> > ErrorOr.h:
>> >   /// \brief Return false if there is an error.
>> >   LLVM_EXPLICIT operator bool() const {
>> >     return !HasError;
>> >   }
>>
>> Oops, I misinterpreted "An implicit conversion to bool provides a way to
>> check if there was an error." - I'll probably clarify that RSN. In any
>> case, isn't the added check here redundant then?
>>
>
> Yep, looks redundant to me.
>
>
>>
>> Maybe it would avoid confusion to use this (fairly common) pattern:
>>
>>   llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
>>       ConfigHandler.second((*Text)->getBuffer());
>>   if (std::error_code EC = ParsedOptions.getError()) {
>>
>
> Agreed, this seems fairly canonical.
>
>
>>     // ...
>>   }
>>
>> >
>> >     so !ParsedOptions implies !ParsedOptions.getError().
>> >
>> >     I think you want:
>> >
>> >       llvm::ErrorOr<ClangTidyOptions> ParsedOptions =
>> >           ConfigHandler.second((*Text)->getBuffer());
>> >       if (ParsedOptions) {
>> >         llvm::errs() << "Error parsing " << ConfigFile << ": "
>> >                      << ParsedOptions.getError().message() << "\n";
>> >         ...
>> >
>> >     This obviously changes the behaviour, but the current behaviour
>> doesn't
>> >     make sense.
>> >
>> >     >        continue;
>> >     >      }
>> >     >
>> >     >
>> >     >
>> >     > _______________________________________________
>> >     > cfe-commits mailing list
>> >     > cfe-commits at cs.uiuc.edu
>> >     > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >     _______________________________________________
>> >     cfe-commits mailing list
>> >     cfe-commits at cs.uiuc.edu
>> >     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141104/45f6dd5c/attachment.html>