[clang-tidy] Check request: portability-minmax-macro-defensive

[denzor200]

Needs a check that will find std::min and std::max invocations without surrounding parentheses over the name. The check will suggest to insert parentheses around the name.

BEFORE:

int result = std::min(a, b); 

AFTER:

int result = (std::min)(a, b); 

Without parentheses the code that uses std::min and std::max might lead to compilation error in MSVC:

#include <algorithm> // std::min
#include <windows.h> // min macro

int main() {
    int a = 5, b = 10;
    
    // Compilation error in MSVC: 
    // "error C2589: '(': illegal token on right side of '::'"
    int result = std::min(a, b); 
    
    return 0;
}

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Denis Mikhailov (denzor200)

Needs a check that will find `std::min` and `std::max` invocations without surrounding parentheses over the name. The check will suggest to insert parentheses around the name.

BEFORE:

int result = std::min(a, b); 

AFTER:

int result = (std::min)(a, b); 

Without parentheses the code that uses std::min and std::max might lead to compilation error in MSVC:

#include <algorithm> // std::min
#include <windows.h> // min macro

int main() {
    int a = 5, b = 10;
    
    // Compilation error in MSVC: 
    // "error C2589: '(': illegal token on right side of '::'"
    int result = std::min(a, b); 
    
    return 0;
}

[vbvictor]

I don't think that it's a job for clang-tidy.
I think it's an issue with <windows.h> header and it's symbol pollution.

[vbvictor]

Instead of min and max it can be any templated function that overlaps with some macro that comes from some header (windows.h or not).

And If there is a strong compiler error provided, then it is catchable without clang-tidy.

[vbvictor]

Also, this code fails for both gcc and clang https://godbolt.org/z/6rca8cqM8, so it must be ill-formed, thus not analyzed.

[denzor200]

#define min(a, b) ... - of course we can write this code on gcc and clang, but there is no reason to do it. This is a smelled code and must be banned in code review. That's it.
But unfortunately <windows.h> already has such smelled code and we can't do something with it - we just use windows.h as it is.

There is guidline to use it safely which shortly means "use parentheses" and most of header-only libraries follow it. Like Boost for example: https://github.com/boostorg/beast/pull/2193/files
And this is really importand because the strong compiler error will not be provided - assume the header-only library doesn't use windows.h. Then the issue will be fired on the user-side build, after the library was released!

This is a job for clang-tidy, because clang-tidy's mission to help every C++ programmer in the world. Why we shouldn't help for authors of header-only libraries too?

[denzor200]

Instead of min and max it can be any templated function that overlaps with some macro that comes from some header (windows.h or not).

of course, but min and max are widely known to have such issue. If you know more - feel free to report in this topic. And of course the check will provide an option to customise it to add more problematic functions.