[flang-commits] [flang] [Flang][#212316] Fix -Wunused-template errors under -Werror (PR #218985)

[Eugene Epshteyn via flang-commits]

https://github.com/eugeneepshteyn commented:

Thank you for the change. I made a few suggestions about the code that could be removed. In general, I strongly prefer `[[maybe_unused]]` to making external declarations for no reason. 

Interesting analysis from AI:
--------

Thanks for cleaning this up — most of it is right, and the header changes are worth having on their own merits. Two things before this lands, though: the change doesn't actually finish the job it names, and the `.cpp` half uses the wrong one of two equally effective idioms.

To check this I ran a census of `-Wunused-template` over **all 398 flang translation units**, reusing each TU's exact compile command with only the diagnostic flag changed, and compared against the same tree with this PR's 11 files reverted:

| tree | unique sites | diagnostics |
|---|---|---|
| `main` without this PR | 23 | 112 |
| this PR | **2** | 2 |
| this PR + the two changes below | **0** | 0 |

The good news first: the 21 sites you fix map 1:1 onto your hunks — nothing here is speculative, and the PR introduces no new site. `check-flang` is green (4860/4860).

---

### 1. Two sites survive, in the library the issue is actually about

```console
flang/lib/Evaluate/tools.cpp:1487:38: warning: unused function template 'tryBuildSplitSumExpressionTree' [-Wunused-template]
flang/lib/Evaluate/tools.cpp:1537:38: warning: unused function template 'tryBuildSplitSumExpressionTree' [-Wunused-template]
```

Reproduce with the compile command for that TU plus:

```console
-fsyntax-only -Wno-everything -Wunused-template
```

These aren't drift from a moving `main` — `git show bc301f51a38f:flang/lib/Evaluate/tools.cpp` (this PR's own base) already contains them; they arrived with 207371 and 217283. And `FortranEvaluate` is precisely the library whose build dies in issue 212316, so as it stands the reporter will rebuild, hit the same `FortranEvaluate.dir` error, and reopen.

Please **annotate rather than delete** — the three overloads at `:1487`, `:1492` and `:1537` are a mutually recursive set (`:1487` is the catch-all terminator; internal calls at `:1498`, `:1545`; entry at `:1584`), so removing either uninstantiated one breaks compilation:

```c++
// Terminator for the recursive overload set below; not instantiated here.
template <typename T>
[[maybe_unused]] static std::optional<Expr<SomeType>>
tryBuildSplitSumExpressionTree(const T &) {
  return std::nullopt;
}
```

```c++
// Reached only through the overload set above; not instantiated here.
template <common::TypeCategory CAT>
[[maybe_unused]] static std::optional<Expr<SomeType>>
tryBuildSplitSumExpressionTree(const Expr<SomeKind<CAT>> &expr) {
```

I applied exactly these two edits plus the inline suggestions below, re-ran the 398-TU census, and got **0 sites and 0 errors**; `git clang-format` reports no changes needed.

### 2. At namespace scope in a `.cpp`, please keep `static` and annotate

Both remedies silence the warning identically — I checked each shape:

| shape | `static`? | `[[maybe_unused]]`? | warns? |
|---|---|---|---|
| free template, named namespace | yes | no | **yes** |
| free template, named namespace | no | no | no |
| free template, named namespace | **yes** | **yes** | **no** |
| free template, **anonymous** namespace | yes | no | **yes** |
| free template, **anonymous** namespace | **no** | no | **still yes** |
| free template, anonymous namespace | no | yes | no |
| member template, external-linkage class | – | no | no |
| member template, **internal**-linkage class | – | no | **yes** |

Dropping `static` is the one that turns an internal symbol into a weak/`linkonce_odr` one:

```console
$ nm -C x.o | grep GetScope
# static:      t  int Fortran::semantics::GetScope<...>(...)
# non-static:  W  int Fortran::semantics::GetScope<...>(...)
```

That matters because a later same-named template in a sibling TU then silently resolves to the wrong definition, with no diagnostic from compiler or linker:

```c++
// a.cpp
namespace Fortran::semantics {
template <typename T> int GetScope(const T &) { return 111; }
struct A { int i; };
int fromA() { A a{}; return GetScope(a); }
}
```
```c++
// b.cpp — a different definition, same name and signature
#include <cstdio>
namespace Fortran::semantics {
template <typename T> int GetScope(const T &) { return 222; }
struct A { int i; };
int fromB() { A a{}; return GetScope(a); }
int fromA();
}
int main() { using namespace Fortran::semantics;
  std::printf("A=%d B=%d\n", fromA(), fromB()); }
```
```console
$ clang++ -std=c++20 -O0 a.cpp b.cpp -o t && ./t
# with `static`:      A=111 B=222   (correct)
# without `static`:   A=111 B=111   (wrong, silently)
```

Nothing is instantiated today, so this is a future collision mode rather than a live bug — but `[[maybe_unused]] static` costs one token and avoids it, and it's what the project already does:

* `llvm/lib/IR/SafepointIRVerifier.cpp:269-273` (commit `71a19716198a`, "[IR] Remove unused and mark debug-only verifier templates (NFC)", 202975) uses exactly `[[maybe_unused]] static` with a comment naming this warning;
* it's what **this PR already does** at `flang/lib/Lower/IO.cpp:129,136`;
* `llvm/docs/CodingStandards.md` asks that file-scope helpers have their visibility "restricted to the current translation unit", and `flang/docs/C++style.md` says "Prefer `static` functions ... in source files";
* clang's own note on the adjacent diagnostic is *"declare 'static' if the function is not intended to be used outside of this translation unit."*

Inline suggestions on the two affected sites are below. One is dead outright and I think should just go (see the `resolve-directives.cpp` comment).

### 3. A couple of corrections to the description

> With clang builds that enable this warning by default and -DFLANG_ENABLE_WERROR=ON, these become fatal errors.

`-Wunused-template` isn't on by default, and no upstream configuration enables it:

```console
clang, no flags                                        -> 0
clang -Wall -Wextra -Wcast-qual -Wimplicit-fallthrough -> 0   # what LLVM actually passes
clang -Weverything                                     -> 4
g++  -Wall -Wextra                                     -> 0   # GCC has no such flag; it rejects it
```

`HandleLLVMOptions.cmake` never adds it, and `FLANG_ENABLE_WERROR` adds only `-Werror` — so the flag is coming from the reporter's own flags. Worth saying so, since the text is permanent history. Two smaller ones: the bullets file `CUFAllocationConversion.cpp` under the `[[maybe_unused]]` group, but the diff also drops `static` there; and unqualified "No functional change" is a bit strong — compiler behaviour is unchanged, but the header hunks do change specialization linkage and symbol visibility. Something like "no change in compiler behaviour; symbol linkage of the affected templates changes as described" would be accurate.

### What I'm explicitly *not* asking you to change

The header hunks (`DirectivesCommon.h`, `PFTBuilder.h`, `RTBuilder.h`, `fold-reduction.h`) are correct and are an improvement independent of the warning — `static` on a namespace-scope template in a header forces a private copy per includer and makes every non-instantiating includer warn.

I also checked the scariest failure mode for `fold-reduction.h`, since its accumulators are per-TU: a de-`static`'d header template instantiated with an **anonymous-namespace type argument** still gets *internal* linkage, so no cross-TU collision is possible. Verified on clang 21, clang 18 and g++ 13. And your per-overload choice in `DirectivesCommon.h` is exactly right — `:86` stays `static` because it genuinely *is* instantiated everywhere (`PeelConvert::visit_with_category` calls `AsRvalueRef(expr.left())` on a `const Convert<...> &`, and `const Expr<Operand<0>> &left() const` makes the `const T &` overload the more specialized match). The only nit there is that the trio now has mixed linkage decided by which overload happened to warn; dropping `static` from `:86` too would make it uniform.

Finally: nothing in-tree enables `-Wunused-template`, so this cleanup can silently rot — as it already did between this PR's base and now. A lit test isn't the right vehicle, but an opt-in cmake option or a warning-clean bot would be worth a follow-up once the tree is actually clean.


https://github.com/llvm/llvm-project/pull/218985