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

[Alexander Kornienko]

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

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

ClangTidyOptions consists of Optionals and one map, so when default
initialized, it's actually empty. However, checking for this state would
require either an equality operator or a dedicated empty() method. Not a
big deal, but the ErrorOr solution seems more straightforward. If we
consider this state of ErrorOr illegal, we should change the ErrorOr class
to avoid it.


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