Change all of FPBits re-declaration to FPB instead.
See https://github.com/llvm/llvm-project/issues/57719#issuecomment-1333900338
Details
- Reviewers
michaelrj sivachandra
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you explain how some of the use cases here are different from https://godbolt.org/z/arvG69d79.
I'm not so sure why there were warnings by gcc in the github issue, but not in godbolt example. My guess is that they are from the constructors:
- Without using FPBits = FPBits<float>, something like FPBits(1.0) will be understood as FPBits<double>(1.0)
- With using FPBits = FPBits<float>, FPBits(1.0) now will be FPBits<float>(1.0) from implicit conversion.
These warnings seem to be inconsistent between different compilers and maybe different versions, but I think it's good to clean them up to prevent ambiguity.
I think the best understanding of the problem comes from these two links:
clang allows it: https://godbolt.org/z/z5rWhzKET
gcc complains: https://godbolt.org/z/T6dvxTWKv
GCC's error message is actually very suggestive. IIUC, it is saying that we are changing the meaning of the type named S within the scope of the U (it is easy to see the problem if U were the global namespace). I think the older version of the code linked to in the bug had this problem: https://github.com/llvm/llvm-project/blob/d883a4ad02d867e7037bd4ec342016c402484148/libc/src/__support/FPUtil/except_value_utils.h
IMHO, it is very subtle. The problem code redefined the type names within the namespace __llvm_libc::fputil. But, I think the current code does not have any such instances - the renames are happening in the namespace __llvm_libc. So, my guess is that that is why we are not seeing similar errors with GCC anymore. Considering that we do have a GCC bot, do we really want to "fix"?
I think you're right! As long as the gcc bot is not complaining, this patch is not needed.