clang-tidy warning: do not use "using namespace" from GENERATE macro in user code

[michaelvlach]

Describe the bug
The specific check link is here: https://releases.llvm.org/8.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/google-build-using-namespace.html The reasons why this is not good is clear I think as it is approximately never you want to import everything from a namespace to your current namespace.

In this particular case I think the better alternative would be another macro like CATCH_GENERATE_USING that would contain all symbols that make sense withing the call to GENERATE, like:

#define CATCH_GENERATE USING \
    using Catch::Generators::as; \
    using Catch::Generators::take; \
\\etc.

Not only will that do away with the warning but it will also help in IDE where it will show only relevant symbols and not literally everything in namespace Catch::Generators (which is a lot).

Expected behavior
"using namespace" is not used.

Reproduction steps
Run clan-tidy with google-build-using-namespace enabled or use IDE integration that performs such static analysis in real time and that includes that check (I am using Qt Creator).

Platform information:

• Catch version: v2.10.2

Additional context
A workaround is to append //NOLINT(google-build-using-namespace) after the definitions of the GENERATE macro lines. That suppresses the warning.

[BillyDonahue]

The workaround to put NOLINT around the GENERATE macros feels like the right answer.

I don't agree with the idea that the using directive is obviously universally harmful, and I think the clang-tidy check is overzealous and should be narrowed down. Sometimes it's necessary and proper to use a namespace directive (like for UDLs), but it has to be done carefully. The namespace being imported must be carefully controlled, like std::literals or std::placeholders or std::rel_ops, AND the using directive should be confined to an appropriately small scope.

Even the clang-tidy check allows them, but only for specially blessed namespaces. I think this shows an unwarranted bias towards treating the standard library as special. What about all the other libraries in the world that define UDLs, like fmt::literals?

https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/google/UsingNamespaceDirectiveCheck.cpp#L38

In the case of the Catch::Generators, I think it would be nice if there was a special namespace with JUST generator names designed for importing, containing only a few using decls. All implementation details should be in another namespace. This is good hygiene and will help with your IDE UI problem.

[michaelvlach]

I mostly agree although my (personal) issue with using namespace x; is that it loses information about where does the particular symbol come from. I would even like it better if Catch2 opted to have those names in catch namespace rather than nested in catch::generators and then imported for "convenience". Consider

const auto data = GENERATE(as<std::string>{}, "Hello", "World", "!");

vs hypothetical:

const auto data = GENERATE(catch::as<std::string>{}, "Hello", "World", "!");

Yeah I may have saved 7 key strokes in the first case but now readers of the code will wonder if the as is my thing or Catch2 thing or wherever it comes from. And sometimes it might not be so obvious. The second one is just obvious and I like that.

Anyhow, I solved it for myself with NOLINT and if the public stuff in catch::generators ever gets its own namespace (even via using using namespace) I would be delighted.

[horenmar]

On the other hand, consider a more complex usage:

const auto data = GENERATE(Catch::take(100,
                                       Catch::filter([](int i) { return i % 2 == 1; },
                                                     Catch::random(-100, 100))));

where the namespacing starts to introduce a lot of noise.

Anyway, for the v2 branch, the solution will definitely be to just suppress the lint. For further development, I'll think about adding a special namespace for the generator helpers, but explicitly listing all the helpers with using Catch::Generators::foo is much more expensive in terms of maintenance than just using a full namespace.