On Tue, Nov 4, 2014 at 4:23 PM, Alexander Kornienko <[email protected]> wrote:
> On Tue, Nov 4, 2014 at 4:11 PM, David Blaikie <[email protected]> wrote: > >> On Tue, Nov 4, 2014 at 4:07 PM, Alexander Kornienko <[email protected]> >> 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 <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Tue, Nov 4, 2014 at 12:23 PM, Justin Bogner <[email protected]> >>>> wrote: >>>> >>>>> David Blaikie <[email protected]> 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 >>>>> > > [email protected] >>>>> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> > _______________________________________________ >>>>> > cfe-commits mailing list >>>>> > [email protected] >>>>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
