[clang-tidy] Unclear purpose/usefulness/actionability of `portability-template-virtual-member-function`

[carlosgalvezp]

I came across the portability-template-virtual-member-function check and don't really understand how the check is useful or actionable. It's not clear as a user what I'm supposed to do to "fix" the code and silence the warning.

The example in the docs shows that there's a compiler error, but that compiler error is not visible in GCC or Clang because the function is not instantiated. It is however visible in MSVC because it is instantiated.

I see two possible scenarios:

• I only ever use GCC/Clang, then I can just disable the check.
• I use both GCC/Clang and MSVC, in which case I'll get a compiler error from MSVC, prompting me to fix the compiler error (that goes unnoticed by GCC). Once I fix the compiler error, clang-tidy still complains, even though I've resolved the actual issue.

So I personally do not see the value in this check, it only bans the usage of virtual member functions in class templates. I would vote for removing it altogether, or at the very least improving the docs to show how people are supposed to fix their code to comply with the check.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Carlos Galvez (carlosgalvezp)

I came across the `portability-template-virtual-member-function` check and don't really understand how the check is useful or actionable. It's not clear as a user what I'm supposed to do to "fix" the code and silence the warning.

The example in the docs shows that there's a compiler error, but that compiler error is not visible in GCC or Clang because the function is not instantiated. It is however visible in MSVC because it is instantiated.

I see two possible scenarios:

• I only ever use GCC/Clang, then I can just disable the check.
• I use both GCC/Clang and MSVC, in which case I'll get a compiler error from MSVC, prompting me to fix the compiler error (that goes unnoticed by GCC). Once I fix the compiler error, clang-tidy still complains, even though I've resolved the actual issue.

So I personally do not see the value in this check, it only bans the usage of virtual member functions in class templates. I would vote for removing it altogether, or at the very least improving the docs to show how people are supposed to fix their code to comply with the check.

[vbvictor]

CC @isuckatcs as author.

[vbvictor]

We have checks with similar semantics: portability-avoid-pragma-once, portability-std-allocator-const. They just ban certain language constructs because they will give errors on some compilers. So I think we should not remove template-virtual-function but maybe give a better name like avoid-template-...

[carlosgalvezp]

But the usage of virtual functions in template classes is not the problem per se, they are perfectly valid in all compilers. It's just that if you have a random compiler error inside them, you may or may not notice it.

The only way to comply with this check is to ban virtual functions in template classes, even though they are supported by all compilers, which does not seem useful. The other checks you mention are indeed things that are compiler-specific or not allowed by the standard.

[vbvictor]

Thank you for the details. Then it seems odd that with this check we only try to avoid a random error in msvc compiler that could be fixed any day (or already fixed)

[carlosgalvezp]

Sorry, what I meant by "random compiler error" is that the user writes arbitrary code that should not compile - I did not mean an MSVC bug/error.

In the provided example:

    virtual void unused() {
        T MSVCError = this;  // illegal code if T cannot be constructed from a CrossPlatformError type
    };

The code inside the unused function is incorrect/illegal, because it assumes any type T can be constructed from a CrossPlatformError, which is not true. If the function was empty, then the code would compile fine on any compiler.

So in my opinion this check would only be hiding real user implementation issues.

[carlosgalvezp]

Do you have any feedback @isuckatcs ?

I ran a search online for seeing the usage and I find some similar comments

#  portability-template-virtual-member-function
#    There is a bunch of these mostly for functions specified as =0. Totally
#    unclear what this is supposed to tell me.

  # portability-template-virtual-member-function,       <-- causes warnings coming from spdlog, not fully understood

  # portability-template-virtual-member-function triggers on pure virtual functions.

(I am aware there is an existing issue about the pure virtual functions. I still question the overall usefulness of this check).

[isuckatcs]

@carlosgalvezp sorry for the late reply!

I created the check after we faced a problem at work, where we developed a cross-platform product and some of us used GCC and some others used MSVC to compile the code and this particular difference between the compilers caught us off-guard.

It is true that you can use both GCC and MSVC on the same machine, however there are certain situations, e.g.: working on macOS, where you cannot compile your code with MSVC, because you can't install it on your system. In this situations if you develop a cross-platform product that must support MSVC as well for any reason, this check could give you a heads up before you overcommit to a solution and realize during testing that it doesn't compile.

I agree that the warning for pure virtual functions shouldn't be reported, and IIRC, it should be fixed by #150290.

Now I wonder if we could make the warning smarter somehow. We still have the body of the function in the AST, so theoretically we could narrow the warning down to only those virtual functions that otherwhise wouldn't compile if they were instantiated. I guess it would make more sense to have the logic for that in Sema though, which would also come with the overhead of analyzin the function body, which we don't do right now IIUC. I wonder what @zygoloid thinks about this change.

On the other hand though, I'm not too pushy about this warning. If the community finds it more annoying than useful, I don't mind removing it completely.

[carlosgalvezp]

we developed a cross-platform product and some of us used GCC and some others used MSVC

IMHO that's not a very good way to develop a product :) A product must be built on a CI machine (with GCC and MVC installed), not on a developer machine. Before merging, every commit should run basic builds and tests in the CI machine (like we do here with the PRs). This would catch the issues you mention before the patch is merged.

Like I said, the problem I see with this check is that it hides real implementation bugs. It does not actually catch portability issues - virtual functions in template classes are supported by all compilers.

It's also unclear how the user is expected to act upon a warning from this check. Remove the virtual function altogether?

[isuckatcs]

A product must be built on a CI machine (with GCC and MVC installed), not on a developer machine. Before merging, every commit should run basic builds and tests in the CI machine (like we do here with the PRs).

Never heard about CI machines and tests. I thought we just force push to prod... preferably Friday afternoon. 🤷‍♀️

Seriously, what I meant is that not everyone used a Windows notebook. I know the CI catches the error, but it's still annoying to investigate and debug the issue. Imagine you work on Linux, or macOS, develop your patch, open a PR and the CI build errors out on Windows with MSVC, and now you need to investigate why that happened.

This way we could drop most checks from the portability category, because the CI would catch the errors anyway.

It does not actually catch portability issues - virtual functions in template classes are supported by all compilers.

I think it's 50/50. For non-virtual member functions we know that when they are not used, they are not instantiated either. If you abuse this rule for some reason and do it with virtual methods as well, then you're relying on unspecified behaviour, which is a portability issue. If you don't deliberately abuse the rule, it just happens to be that some functions are not used, it can still be a portability problem.

It's also unclear how the user is expected to act upon a warning from this check.

Yeah, I agree, the check could be more specific on which functions are actually problematic, because they will cause the build to error out on MSVC. In that case the user might fix, or remove them. Now it indeed doesn't really offer a solution.

[carlosgalvezp]

I know the CI catches the error, but it's still annoying to investigate and debug the issue

Fair enough! But isn't that a general problem? Any type of code can compile/run differently on GCC/Clang/MSVC.

Overall, my feeling is that we are in XY problem situation. The "real" problem, in my view, is that there is dead code (unused, untested), which the compiler may or may not choose to compile/instantiate. If all code was built/used/tested, the compiler error would be caught in GCC/Clang, not only on MSVC.

The solution to that problem would be to increase build/test coverage, and improve coverage tools to catch this.

But, I understand that the ideal solution isn't always easy or exists, so a pragmatic solution is to restrict the subset of C++. I'd be OK with keeping the check if we could document it a bit more and clarify for the user what the problem is and what the check is solving.

[isuckatcs]

Fair enough! But isn't that a general problem? Any type of code can compile/run differently on GCC/Clang/MSVC.

I think it depends on WHY the code compiles/runs differently. The root cause can be undefined behaviour, unspecfied behaviour or even a bug in the compiler. In this case the code relies on unspecified behaviour.

An implementation shall not implicitly instantiate a function template, a variable template, a member template,
a non-virtual member function, a member class or static data member of a templated class, or a substatement of a
constexpr if statement, unless such instantiation is required. [...]

It is unspecified whether or not an implementation implicitly instantiates a virtual member function of a class
template if the virtual member function would not otherwise be instantiated.

(https://eel.is/c++draft/temp.inst#11)

I think the general idea is that any developer is safe to assume that compilers behave as the standard requests them. IIUC, this implies that if the goal is to write portable code, the behaviour of the code should be deterministic according to the standard. Since in this case the behaviour is implementation defined, we cannot say it's deterministic when we look at it from a portability perspective.

[carlosgalvezp]

Ok! I see your point, I understand the use case of wanting to avoid unspecified behavior and have a deterministic program regardless of compiler.

I'm happy closing this issue, I'll leave it up to you if you want to add/clarify anything in the docs ;)