njames93 added a comment. In D147357#4256636 <https://reviews.llvm.org/D147357#4256636>, @PiotrZSL wrote:
> As for `takeOptionalValue(std::move(*param));`, it could be added as an > configuration option for PassthroughFunctions or UtilityFunctions. I can > thing about that, that should be easy to implement. That's overcomplicating things, just look for `std::move` and you'll cover 99.9% of instances in real world code. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst:34 +Value extraction using ``operator *`` is matched by default. +Check does not provide auto-fixes. + ---------------- PiotrZSL wrote: > njames93 wrote: > > Any reason for this limitation, given in most cases the fix is to just > > remove the `*` or `.value()` call > Sometimes problem is not with optional but with called function that take > optional when it could take value. > At least for such issues I run, still probably could be safe to introduce > such auto-fix to just remove */.value(). > But then question would be if it should be bugprone or readability check. > For me like 10% of these issues show some possible bug around that code. That's a good point, if they are hiding a bug we don't want to auto fix it. Perhaps the fix should be added as a note ``` note: Remove the call to '%FuncName%' to silence this warning note: Remove '*' to silence this warning ``` ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp:16 + }; +} + ---------------- PiotrZSL wrote: > njames93 wrote: > > It'd be good to have tests demonstating `boost::optional` and > > `absl::optional` as well as the (non standard) get accessor > I added them to config just to add them, as they should work, I could add > some tests for them, but behavior should be same... On the subject of boost(unsure about absl as I've never used that library) Would you ever add support for `T& boost::get(boost::optional<T> &)` or its const ref cousin? ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp:37 + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] +} + ---------------- What happens with this pathological case `takeOptionalValue(param.operator*());` Looks like it isn't being handled, should probably handle(and test) for it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147357/new/ https://reviews.llvm.org/D147357 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
