Remove using namespace std / using-declarations from headers - #364
Conversation
Expand the terse "No using namespace in headers" bullet into a dedicated "Namespace using" section: - Headers: ban namespace-scope using-directives and using-declarations; class/function-body using and alias-declarations remain allowed. - .cpp: prefer specific using-declarations; file-scope using namespace is permitted but discouraged. - Never rely on a using supplied transitively by an included header. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J8hXn6F1s3ybUogy85sCH
Header code no longer relies on `using namespace std;` (or a namespace-scope `using std::chrono::time_point;`); the affected declarations are now fully qualified. Cleaned headers: - system/system.hpp, system/thread_mgr.hpp, system/time_stat.hpp, system/timer.hpp - tests/TestTimer.hpp Translation units that had been leaning on those headers' transitive `using` now declare what they need locally: specific `using std::...;` declarations in the system/*.cpp and init.cpp printing code, and point qualification of `std::vector` / `std::chrono::time_point` elsewhere. Full `bazel build //...` and `bazel test //...` (93 tests) pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J8hXn6F1s3ybUogy85sCH
There was a problem hiding this comment.
🟡 Changes recommended
library/src/system/timer.hpp uses clock_t but does not include <ctime>, which can break header self-containment/portability across standard libraries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes header-level namespace pollution by eliminating using namespace std; and namespace-scope using X::name; from several headers, then updating call sites and the C++ style guide to prevent recurrence.
Changes:
- Fully qualified standard-library types in affected headers (e.g.,
std::string,std::vector,std::chrono::time_point). - Updated translation units/tests that previously relied on transitive
usingfrom headers by adding localusing std::...;or explicitstd::qualification. - Expanded
.github/instructions/cpp.instructions.mdwith a dedicated “Namespaceusing” section clarifying header vs.cpprules.
File summaries
| File | Description |
|---|---|
| library/src/system/system.hpp | Removes header using namespace std; and qualifies STL usage in declarations. |
| library/src/system/thread_mgr.hpp | Removes header using namespace std; and qualifies std::vector/std::string. |
| library/src/system/time_stat.hpp | Removes header using namespace std; and qualifies std::string return types. |
| library/src/system/timer.hpp | Removes header using namespace std; / using std::chrono::time_point; and qualifies std::string/std::chrono::time_point. |
| library/tests/TestTimer.hpp | Removes namespace-scope using std::chrono::time_point; and fully qualifies std::chrono::time_point. |
| library/src/init.cpp | Adds local using std::...; declarations to avoid relying on header-provided using. |
| library/src/solve_board.cpp | Qualifies std::vector in helper signature to avoid transitive using. |
| library/src/calc_tables.cpp | Qualifies std::vector in helper signature to avoid transitive using. |
| library/src/system/system.cpp | Adds local using std::...; declarations for formatting/containers used in the TU. |
| library/src/system/thread_mgr.cpp | Adds local using std::...; declarations used by output/formatting code. |
| library/src/system/time_stat.cpp | Adds local using std::...; declarations used by formatting code. |
| library/src/system/time_stat_list.cpp | Adds local using std::...; declarations used by formatting code. |
| library/src/system/timer.cpp | Adds local using std::...; declarations and qualifies chrono types where needed. |
| library/src/system/timer_group.cpp | Adds local using std::...; declarations used by formatting/string building. |
| library/src/system/timer_list.cpp | Adds local using std::...; declarations used by printing code. |
| library/tests/TestTimer.cpp | Qualifies std::chrono::time_point in end() to avoid relying on header using. |
| library/tests/testcommon.cpp | Adds using std::left/right; alongside existing local using block. |
| .github/instructions/cpp.instructions.md | Documents the “Namespace using” policy in detail. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- timer.hpp: include <ctime> explicitly for clock_t (member syst0) instead of relying on a transitive include via <chrono> (Copilot review) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J8hXn6F1s3ybUogy85sCH
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently remove header-level namespace pollution while updating dependent translation units accordingly, and the updates are low-risk and stylistically aligned with the documented conventions.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ild) The DDS_SCHEDULER-only Scheduler::PrintTiming() used unqualified string, ofstream, setw, setprecision and fixed, which had resolved via a transitive `using namespace std;` from the now-cleaned system headers. Add function-local using-declarations so the scheduler=true build compiles again. Verified: `bazel build --define=scheduler=true //library/src:dds`, `--define=ab_stats=true`, and `bazel build/test //...` all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J8hXn6F1s3ybUogy85sCH
There was a problem hiding this comment.
🔵 Needs a closer look
A few headers/translation units still rely on unqualified clock_t/clock() (not guaranteed outside std::), which can cause non-portable compile failures after removing header-level using.
Review details
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
library/tests/TestTimer.cpp:97
- Use
std::clock_t/std::clock()consistently in this block as well (and update thestatic_casttype) to avoid depending on a globalclock_tname.
std::chrono::time_point<Clock> user1 = Clock::now();
clock_t sys1 = clock();
duration<double, std::milli> d = user1 - user0_;
const long tuser = static_cast<long>(d.count());
library/src/system/timer.cpp:66
- Prefer
std::clock_t/std::clock()here so this .cpp does not rely on a globalclock_torclock()being injected via C headers; this becomes more important after removing transitiveusingfrom headers.
library/src/system/timer.hpp:38 - After removing header-level
using namespace std,clock_tshould be qualified asstd::clock_tfor standard compliance;<ctime>only guaranteesstd::clock_t, not a globalclock_ttypedef on all platforms.
library/tests/TestTimer.cpp:93 - This file now avoids relying on transitive
usingfrom headers;clock_tandclock()should be written asstd::clock_t/std::clock()for standard compliance (including<ctime>only guarantees these names instd::).
library/tests/TestTimer.hpp:47 - This header now avoids namespace-scope
usingdeclarations;clock_tshould similarly be written asstd::clock_tbecause<ctime>only guarantees the typedef instd::, not necessarily in the global namespace.
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
Copilot review 5088220782: <ctime> only guarantees clock_t/clock() in namespace std, not the global namespace. After removing header-level `using`, write them qualified. - timer.hpp / timer.cpp: std::clock_t member, std::clock() calls - TestTimer.hpp / TestTimer.cpp: std::clock_t (member, param, casts), std::clock() calls - test_timer_test.cpp: same, for module consistency bazel build //... , --define=scheduler=true //library/src:dds , and bazel test //... (93 tests) all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J8hXn6F1s3ybUogy85sCH
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently remove header-level namespace pollution and update dependent translation units accordingly without introducing API/behavior changes.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
What
using namespace std;(and one namespace-scopeusing std::chrono::time_point;) appeared at file scope in several headers. That injects names into every translation unit that includes the header, directly or transitively — bad style and a maintenance hazard.Headers cleaned (declarations now fully qualified)
library/src/system/system.hpplibrary/src/system/thread_mgr.hpplibrary/src/system/time_stat.hpplibrary/src/system/timer.hpplibrary/tests/TestTimer.hppThe
using Clock = std::chrono::steady_clock;alias-declarations are left as-is (they are type aliases, not using-directives/declarations).Translation units updated
Several
.cppfiles had been leaning on those headers' transitiveusing. They now declare what they need locally:using std::...;declarations added toinit.cppand thesystem/*.cppprinting/formatting code.std::vector,std::chrono::time_point) incalc_tables.cpp,solve_board.cpp,timer.cpp,TestTimer.cpp.testcommon.cppgainsusing std::left; using std::right;alongside its existingusing std::block.Coding standard
.github/instructions/cpp.instructions.mdgains a dedicated Namespaceusingsection replacing the terse "Nousing namespacein headers" bullet: headers ban namespace-scope using-directives and using-declarations (class/function-bodyusingand alias-declarations still fine);.cppfiles prefer specific using-declarations, with file-scopeusing namespacepermitted but discouraged; never rely on ausingsupplied by an included header.Testing
bazel build //...— passesbazel test //...— 93/93 pass🤖 Generated with Claude Code
Closes #357