3

I'm writing C++ code in CLion 2021.3, which uses clang-tidy checks.

In my code, I have a lightweight reference class; let's say it looks like this:

struct resource_t {
    uint8_t kind;
    int id;
}

Now, when I pass a resource_t object around, I want to pass it by value. There is no reason I would need to employ references to it, or move it, or what-not. However, clang-tidy doesn't like this. So it complains about my functions with take resource_t's by values, and about my passing them by-value to these functions. It always suggests to me to make the parameters const resource_t&'s, and in some cases to consider passing std::move(my_resource) if that's the only use of my_resource in the function.

Now, these warnings are fine for "heavy" classes - but not for this one. Is there any way - with CLion or for clang-tidy generally - to get clang-tidy to tell apart the "heavy" from the "lightweight" case? Or perhaps to whitelist some specific types?

einpoklum
  • 118,144
  • 57
  • 340
  • 684
  • 4
    you should provide also the context where you are passing the object. In addition we have no idea what `kind_t` is. – Ruggero Turra Dec 27 '21 at 14:38
  • What's the benefit to passing by value over passing by reference here? Or are you saying they're equivalent? – JohnFilleau Dec 27 '21 at 14:39
  • 2
    Maybe its a bit pedantic, but why not pass by const& regardles? It perfectly states the intended use, it won't cost you anything (except maybe a little typing), and you won't have different rules for "light-weight" and "heavy-weight" structs. Also if at a later time during refactoring a "light-weight" struct becomes a "heavy-weight" struct you don't have to go through all your code again. – Pepijn Kramer Dec 27 '21 at 14:48
  • @JohnFilleau: by value avoids one indirection by access, aliasing issue. But it does one extra copy. For *small* (`sizeof (T) < sizeof(T*)`) types, the drawback is even not existent. – Jarod42 Dec 27 '21 at 14:48
  • 1
    @Jarod42 • Can the *indirection by access* be optimized away? Is it required? Is it an implementation detail? – Eljay Dec 27 '21 at 14:55
  • 1
    @Jarod42 a reference is not required to be implemented as an indirection. I had filed this problem away as "let the compiler worry about it". Now I'm second guessing if I should be passing by value for small values. – JohnFilleau Dec 27 '21 at 15:00
  • On two of my platforms, pass-by-value of *non resource managing* (NRM) object parameters that are 256 bytes or smaller is a **win**. Pass-by-reference of NRMs that are 512 bytes or larger is a win. 256-512 byte range is about a wash. (All parameters collectively contribute to the cutoff.) This "rule of thumb" is only applicable to two of my platforms; I've not figured the break even for my other four platforms. (I was looking into Coverity complaints about large parameters. Similar-ish to what clang-tidy is complaining about.) – Eljay Dec 27 '21 at 15:05
  • @JohnFilleau: if compiler has code visible, it might indeed remove difference between pass by value or pass by reference (inlining the code, or change to the most appropriate). but for code non visible, without LTO, I don't see how compiler might avoid indirection. – Jarod42 Dec 27 '21 at 15:25
  • @RuggeroTurra: That's not the point, but changed the type to make it even simpler. – einpoklum Dec 27 '21 at 18:29
  • @JohnFilleau: Do you ever pass (plain) pointers by reference? Do you pass spans by reference? It's the same thing. I don't want people to have a reference to a `resource_t` instance, I wanted them to have a `resource_t`. – einpoklum Dec 27 '21 at 18:30
  • @Eljay: TBH, and if you use `__restrict`, a decent compiler should be able to convert a pass-by-ref to pass-by-value in such cases. – einpoklum Dec 27 '21 at 18:32
  • @eink if it's 4 bytes or smaller I pass by value. I think what I'll do going forward is const references for header-defined functions or all parameters that take 4 or more bytes. If I want the naked pointer to be modifiable I pass by non-const reference. I don't know what a span is. – JohnFilleau Dec 27 '21 at 18:34
  • @JohnFilleau: So, on 64-bit platforms, you always pass pointers by reference then? I suppose you could do that, but this is an extremely rare custom. The answer to "why" is that it's not what I mean. I want to say what I mean. – einpoklum Dec 27 '21 at 18:35
  • @eink Sometimes yeah. Why? I'm not calling you wrong. This is normally something I don't think about, so now I'm thinking about it. – JohnFilleau Dec 27 '21 at 18:36
  • "If the parameter is not const, only copied or assigned once and has a non-trivial move-constructor or move-assignment operator respectively the check will suggest to move it.". I cannot reproduce this warning with your type, which doesn't have a move ctor. If your *actual* type has one, remove it. – n. m. could be an AI Dec 27 '21 at 19:04
  • @n.1.8e9-where's-my-sharem.: My actual class is more complicated, and has a non-trivial move-ctor... and I don't want it moved. – einpoklum Dec 27 '21 at 19:10
  • 1
    So your class doesn't actually look like you describe. It has a move constructor. The question is misleading. – n. m. could be an AI Dec 28 '21 at 04:36

1 Answers1

2

Clang tidy has dedicated syntax to silence specific warnings on specific lines. I imagine the warning you're referring to is performance-unnecessary-value-param. You can obviously globally disable that check, but it would leave your code exposed (assuming you trust the tool elsewhere but not in this specific case).

So to disable it locally, say a specific function declaration you have to decorate your code with directives to ignore the warning. For example you can silence checks ending with -performance-unnecessary-value-param for all lines between the BEGIN and END

// NOLINTBEGIN(*-performance-unnecessary-value-param)
void foo(resource_t iKnowBetter);
// NOLINTEND(*-performance-unnecessary-value-param)

Source

EDIT: Drawing from the comments, I gather that specific types can also be whitelisted like so:

  1. Add a .clang-tidy configuration file to your project (I use this one for example).
  2. Whitelist your type like so: 
      CheckOptions:
- key:   performance-unnecessary-value-param.AllowedTypes
  value: resource_t

With all the above been said note one last thing regarding your specific warning:

The check is only applied to parameters of types that are expensive to copy which means they are not trivially copyable or have a non-trivial copy constructor or destructor.

which means the structure you posted shouldn't trigger this warning, unless there's more to kind_t. In any case tweaking the structure to make sure it adheres to the "trivially copyable" requirement might be the cleaner solution.

Nikos Athanasiou
  • 29,616
  • 15
  • 87
  • 153
  • But this doesn't happen in a specific function, it happens in hundreds, or thousands, of uses of `resource_t`. – einpoklum Dec 27 '21 at 18:35
  • 1
    The documentation mentions suppressing this warning for specific types (needs to be set up in clang-tidy options). – n. m. could be an AI Dec 27 '21 at 19:06
  • @n.1.8e9-where's-my-sharem. Correct, I've updated the answer – Nikos Athanasiou Dec 27 '21 at 21:53
  • @einpoklum updated the answer. If you have thousands of uses of the type you might want to consider the warnings clang-tidy is emitting though. As mentioned a trivially copy-able type shouldn't give such a warning. In any case globally muting this warning for your type is in fact possible – Nikos Athanasiou Dec 27 '21 at 21:54
  • this is not helpful because you have to tell it for each line where it is used, not for the type – Enerccio Aug 31 '22 at 12:37
  • 2
    @Enerccio The method to disable the warning for a specific type is also mentioned. Read the answer past the **EDIT** – Nikos Athanasiou Aug 31 '22 at 20:07