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

[David Blaikie]

On Tue, Nov 4, 2014 at 4:07 PM, Alexander Kornienko <alexfh at google.com>
wrote:

> 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).
>

Yeah, that seems kind of... not good.

I'd model this as ErrorOr<Nullable<ClangTidyOptions>> if ClangTidyOptions
doesn't have a valid null state that you could use for (3).


> 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/f14c1fc8/attachment.html>