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