microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.2k stars 1.51k forks source link

tests: Should we follow stylistic conventions more consistently? #1110

Open StephanTLavavej opened 4 years ago

StephanTLavavej commented 4 years ago

In test code, we've historically been less strict about enforcing global stylistic conventions. We generally expect consistency within a test, but different tests vary in:

Historically, this seemed reasonable because it allowed each maintainer to write test code in their most preferred style, and we generally don't treat the test code as a unified whole (we generally write a test, and then it stays unchanged unless we need to fix/extend it). However, this makes things somewhat confusing for contributors, since we don't have consistent precedents to follow.

StephanTLavavej commented 4 years ago

My preferences are:

AlexGuteniev commented 4 years ago

My habit from supporting bloated code bases compiled into single binary (or even multiple binaries running together and sharing headers): never using namespace in namespace scope. (I also tend to qualify Windows API calls with :: for the same) That said, I see the point to adhere to using namespace std; rule in STL tests.

class/typename are different things to me. class is something that meets certain concept, like policy or iterator. typename is something more broad, like a type in container, or a functor that only meets its call requirement.

In const west convention, which is more western - const or volatile?

CaseyCarter commented 4 years ago

I am 100% certain that we should follow product code conventions in tests to make it easier to move around in the codebase. I'm 80% certain that we should follow the style conventions of the Standard Library specification where possible in product code to make it easier for folks to validate against the spec.

I'm perfectly fine with using namespace std; in conforming modes but it can occasionally make me violently ill in MSVC's permissive mode when used at namespace scope. (Read this as "not my preference but tolerable.")

Aside: we've been going out of our way to using namespace std in Ranges tests, both because I've been working ahead of this discussion, and to provide extra dribs and drabs of validation for the design intent that using ranges::meow; at function scope overrules using namespace std and/or ADL lookup for meow(/**/) finding std::meow.

I used to be a typenamer before experimentally switching to the unambiguous "struct for classes, class for type parameters, typename to disambiguate" style which I now prefer, but again I think we should follow the Standard's style for these questions rather than anyone's personal preference.

AdamBucior commented 4 years ago

I also think that tests should follow the same conventions as product code. I definitely prefer using namespace std;. In my code I always use typename.

StephanTLavavej commented 2 years ago

2 years later, we've talked about this at the weekly maintainer meeting, and we've decided to go with the outline mentioned in https://github.com/microsoft/STL/issues/1110#issuecomment-666033003. We'll consider cleanup PRs to make the tests more consistent, and such PRs can be done on a file-by-file basis - no need for an enormous overhaul-the-world PR (which would be a headache both in terms of reviewing and merge conflicts).

Note that a few tests will continue to be special: the any, optional, and variant tests are derived from LLVM sources and thus should not be modified here. Additionally, tests which explicitly mention that they're intentionally avoiding using namespace std; (like the test for User-Defined Literals) have good reasons to do so, so such comments should not be removed. Finally, as usual, we refrain from making significant changes to the legacy tr1 test suite.