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