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