[PATCH] [clang-tidy] Add support for boolean check options.

Marek Kurdej curdeius at gmail.com
Mon Oct 6 01:52:14 PDT 2014


We can generalize go a step further `get()` and `store()` to all types that specialize `llvm::yaml` traits.
This could remove the code duplication and simplify the creation of clang-tidy checks having options of enumeration type.

================
Comment at: clang-tidy/ClangTidy.h:50
@@ -49,3 +49,3 @@
   /// \p Default.
-  std::string get(StringRef LocalName, std::string Default) const;
+  std::string get(StringRef LocalName, const char *Default) const;
 
----------------
What's the reason for changing `std::string Default` to `const char* Default`?
It "breaks" somehow the symmetry of `T get(StringRef, T)`, i.e. taking `T` type as Default and returning `T`.

================
Comment at: clang-tidy/ClangTidy.h:73
@@ +72,3 @@
+  /// \p Default.
+  bool get(StringRef LocalName, bool Default) const {
+    std::string Value = get(LocalName, "");
----------------
curdeius wrote:
> Couldn't we just generalize this approach to all the types that specialize `llvm::yaml::XYZTraits` (XYZ being one of Scalar/ScalarEnumeration/...) ?
> 
> An ugly hack (working at least for `enum`s specializing `ScalarEnumerationTraits`) would be:
> 
> ```
>   template <typename T>
>   typename std::enable_if<!std::is_integral<T>::value &&
>                               !std::is_convertible<T, std::string>::value,
>                           T>::type
>   get(StringRef LocalName, T Default) const {
>     std::string Value = get(LocalName, "");
>     T Result = Default;
>     if (!Value.empty()) {
>       std::stringstream Content;
>       Content << LocalName.str() << ": " << Value;
>       llvm::yaml::Input Input(Content.str());
>       Input.setCurrentDocument();
>       Input.mapOptional(LocalName.data(), Result);
>       if (Input.error())
>         Result = Default;
>     }
>     return Result;
>   }
> ```
> 
> The hack is ugly since it uses `Input::setCurrentDocument()` (which should probably be private) and `Input::mapOptional()` and not `Input::operator>>`. That is so, because otherwise we would need creating a type specializing `llvm::yaml::MappingTraits` which would essentially do an operation equivalent to `Input.mapOptional(LocalName.str(), Result);`.
That's essentially the same code as in `llvm/lib/Support/YAMLTraits.cpp::651`.

================
Comment at: clang-tidy/ClangTidy.h:73-80
@@ +72,10 @@
+  /// \p Default.
+  bool get(StringRef LocalName, bool Default) const {
+    std::string Value = get(LocalName, "");
+    if (Value == "true")
+      return true;
+    if (Value == "false")
+      return false;
+    return Default;
+  }
+
----------------
Couldn't we just generalize this approach to all the types that specialize `llvm::yaml::XYZTraits` (XYZ being one of Scalar/ScalarEnumeration/...) ?

An ugly hack (working at least for `enum`s specializing `ScalarEnumerationTraits`) would be:

```
  template <typename T>
  typename std::enable_if<!std::is_integral<T>::value &&
                              !std::is_convertible<T, std::string>::value,
                          T>::type
  get(StringRef LocalName, T Default) const {
    std::string Value = get(LocalName, "");
    T Result = Default;
    if (!Value.empty()) {
      std::stringstream Content;
      Content << LocalName.str() << ": " << Value;
      llvm::yaml::Input Input(Content.str());
      Input.setCurrentDocument();
      Input.mapOptional(LocalName.data(), Result);
      if (Input.error())
        Result = Default;
    }
    return Result;
  }
```

The hack is ugly since it uses `Input::setCurrentDocument()` (which should probably be private) and `Input::mapOptional()` and not `Input::operator>>`. That is so, because otherwise we would need creating a type specializing `llvm::yaml::MappingTraits` which would essentially do an operation equivalent to `Input.mapOptional(LocalName.str(), Result);`.

================
Comment at: clang-tidy/ClangTidy.h:103-104
@@ +102,4 @@
+  /// a \c bool value \p Value to \p Options.
+  void store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+             bool Value) const;
+
----------------
Similarly to above `get()` method, one can generalize the approach.
Another ugly hack would be:

```
  template <typename T>
  typename std::enable_if<!std::is_integral<T>::value &&
                              !std::is_convertible<T, std::string>::value,
                          void>::type
  store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
        T Value) const {
    std::string Text;
    llvm::raw_string_ostream Content(Text);
    llvm::yaml::Output Output(Content);
    Output.beginMapping();
    if (Output.preflightDocument(0))
      Output.mapOptional(LocalName.data(), Value);
    Output.endMapping();

    auto T = Content.str();
    // Skip past ": " between key and value.
    auto Off = T.find(':', LocalName.size());
    assert(Off != std::string::npos);
    T = T.substr(Off + 2, std::string::npos);
    // FIXME(mkurdej): Strip enclosing apostrophes "'" if present.
    Options[NamePrefix + LocalName.str()] = T;
  }
```

http://reviews.llvm.org/D5602






More information about the cfe-commits mailing list