[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:23 PM, Alexander Kornienko <alexfh at google.com>
wrote:

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

(sorry, yes, s/Nullable/Optional/)


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

explicit operator bool - but yeah, if it's not a sensible thing for the
type to have, there's no reason to add it.

(On the other hand.... is it valid for a ClangTidyOptions to be returned in
that state? If not, I might be inclined to add it - same way returning an
int from a function sometimes uses 0 for "no result" if a 'no result'
result is needed and zero isn't needed in the valid set)


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

I think that's something worth considering/doing. 'successful' errors are
kind of a scary no-man's land that's infamous on Windows & I'd rather not
repeat... but perhaps other people with more experience in such things
might indicate that this is a good/valid/important state.


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