clang-tidy suggests CamelCase for a single variable (when the config is camelBack)

[joker-eph]

With clang-tidy freshly built:

bin/clang-tidy -p build mlir/lib/Analysis/FlatLinearValueConstraints.cpp --checks=-*,readability-identifier-naming 

2397 warnings generated.
mlir/lib/Analysis/FlatLinearValueConstraints.cpp:1306:38: warning: invalid case style for parameter 'var' [readability-identifier-naming]
 1306 |       [](const std::optional<Value> &var) { return var.has_value(); }));
      |                                      ^~~           ~~~
      |                                      Var           Var
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
   93 |      (static_cast <bool> (expr)                                         \
      |                           ^
mlir/lib/Analysis/FlatLinearValueConstraints.cpp:1310:38: warning: invalid case style for parameter 'var' [readability-identifier-naming]
 1310 |       [](const std::optional<Value> &var) { return var.has_value(); }));
      |                                      ^~~           ~~~
      |                                      Var           Var
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
   93 |      (static_cast <bool> (expr)                                         \
      |                           ^
Suppressed 2401 warnings (2395 in non-user code, 6 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

But see the surrounding code, it's all camelBack:

static void mergeAndAlignVars(unsigned offset, FlatLinearValueConstraints *a,
                              FlatLinearValueConstraints *b) {
  assert(offset <= a->getNumDimVars() && offset <= b->getNumDimVars());

  assert(llvm::all_of(
      llvm::drop_begin(a->getMaybeValues(), offset),
      [](const std::optional<Value> &var) { return var.has_value(); }));

  assert(llvm::all_of(
      llvm::drop_begin(b->getMaybeValues(), offset),
      [](const std::optional<Value> &var) { return var.has_value(); }));

  SmallVector<Value, 4> aDimValues;
  a->getValues(offset, a->getNumDimVars(), &aDimValues);

I don't know why the callback here was singled out?

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Mehdi Amini (joker-eph)

With clang-tidy freshly built:

bin/clang-tidy -p build mlir/lib/Analysis/FlatLinearValueConstraints.cpp --checks=-*,readability-identifier-naming 

2397 warnings generated.
mlir/lib/Analysis/FlatLinearValueConstraints.cpp:1306:38: warning: invalid case style for parameter 'var' [readability-identifier-naming]
 1306 |       [](const std::optional<Value> &var) { return var.has_value(); }));
      |                                      ^~~           ~~~
      |                                      Var           Var
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
   93 |      (static_cast <bool> (expr)                                         \
      |                           ^
mlir/lib/Analysis/FlatLinearValueConstraints.cpp:1310:38: warning: invalid case style for parameter 'var' [readability-identifier-naming]
 1310 |       [](const std::optional<Value> &var) { return var.has_value(); }));
      |                                      ^~~           ~~~
      |                                      Var           Var
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
   93 |      (static_cast <bool> (expr)                                         \
      |                           ^
Suppressed 2401 warnings (2395 in non-user code, 6 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

But see the surrounding code, it's all camelBack:

static void mergeAndAlignVars(unsigned offset, FlatLinearValueConstraints *a,
                              FlatLinearValueConstraints *b) {
  assert(offset <= a->getNumDimVars() && offset <= b->getNumDimVars());

  assert(llvm::all_of(
      llvm::drop_begin(a->getMaybeValues(), offset),
      [](const std::optional<Value> &var) { return var.has_value(); }));

  assert(llvm::all_of(
      llvm::drop_begin(b->getMaybeValues(), offset),
      [](const std::optional<Value> &var) { return var.has_value(); }));

  SmallVector<Value, 4> aDimValues;
  a->getValues(offset, a->getNumDimVars(), &aDimValues);

I don't know why the callback here was singled out?

[badwriter123]

Hey @joker-eph ,
I notice that the main issue is that the ParameterCase in the .clang-tidy is set to CamelCase ,

--> - key: readability-identifier-naming.ParameterCase
value: CamelCase

So the fix for this would be to change the ParameterCase to CamelBack and explicitly add ConstantParameterCase and set it to CamelBack , this would ensure a fix for both parameters like int x, float y , etc and for parameters declared as const , eg,. const T& var, const int n .

What do you think of this fix ??

[vbvictor]

In mlir/.clang-tidy:

llvm-project/mlir/.clang-tidy

Lines 57 to 58 in ecd8754

	- key: readability-identifier-naming.ParameterCase
	value: camelBack

So this config should be taken when analyzing files in mlir, but maybe ConstantParameterCase will help

[badwriter123]

Yeah , This handles the parameters like int x, float y etc,

- key:             readability-identifier-naming.ParameterCase
   value:           CamelBack

Adding this explicitly (ConstantParameterCase) handles cases like const T& var, etc. So ConstantParameterCase will surely help.

  - key:             readability-identifier-naming.ConstantParameterCase
    value:           camelBack

[vbvictor]

This behavior still seems strange because in docs it says

Many configuration options are available, in order to be able to create different rules for different kinds of identifiers. In general, the rules are falling back to a more generic rule if the specific case is not configured.

So this must have fallen back to ParameterCase when there is no ConstantParameterCase. Or maybe felt to a different option

[badwriter123]

Yeah exactly, clang-tidy has different naming rules for different things like for variables, parameters, constants, etc.
if we don’t set a rule for something, it “falls back” to a more generic rule.

like for const parameters, clang-tidy does not reuse the ParameterCase and instead, it falls back to the constant rule (ConstantCase), which is a default to CamelCase.

and that’s why const T& var got flagged → it was being checked as a “constant,” and not as a normal parameter.

so the fix would be to explicitly set ConstantParameterCase = camelBack so const parameters follow the same style as normal ones.

[carlosgalvezp]

ParameterCase is the more general rule of ConstantParameterCase.

But yes, it doesn't hurt to set them all and avoid relying on implicit, undocumented rules :)

[joker-eph]

ParameterCase is the more general rule of ConstantParameterCase.

So to be clear: it is not a configuration problem right?

When I try to add ConstantParameterCase explicitly to the MLIR config, the problem continues to exist.

[badwriter123]

ok so If changing ConstantParameterCase to camelBack didn't fix the issue, there might be something else going on, and given that there is no configuration issue, the .clang-tidy has cache that is not being cleared properly .

If still the issue persists i guess there is a bug in how .clang-tidy classifies lambda parameters.

[joker-eph]

I did some experiment, it seems related to the fact that this is wrapped in a macro.
In the example above if I remove the surrounding assert, clang-tidy is happy.

[joker-eph]

Minimal example:

#define X(Y) Y
X(int foo(int v){})
int bar(int v){}

Here clang-tidy will complain about the variable in the definition of foo but not bar.

[badwriter123]

Yeah this is exactly a .clang-tidy bug, when .clang-tidy encounters macros it goes berserk , loses context and falls back to the default rules.

I guess the fix would be to tell .clang-tidy to ignore macros or add a macro-specific ignore pattern so that .clang-tidy skips naming checks.

[joker-eph]

Why isn't the fix to address:

clang-tidy encounters macros it goes berserk , loses context and falls back to the default rules.

That is: don't go berserk and respect the rules?