[CXX-95] Client facing headers should not include windows.h Created: 13/Mar/13 Updated: 07/Jan/15 Resolved: 07/Jan/15 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | Implementation |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Andrew Morrow (Inactive) | Assignee: | Unassigned |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | cxxmove, legacy-cxx, windows | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||
| Description |
|
If you use the C++ client driver headers, you automatically include windows.h, and get WIN32_LEAN_AND_MEAN set, via our windows_basic.h header. Our driver shouldn't drag all of windows.h into scope just to talk to the database. windows_basic.h should only be included in .cpp scope, not header scope. See HELP-78 for examples of where this causes problems in practice. |
| Comments |
| Comment by Eric Milkie [ 11/Jun/14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
We no longer have client-facing headers in the server code, so we're good there. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 11/Jun/14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
It isn't really relevant in the server, since the header files aren't meaningfully consumed outside of the project. Interestingly, the server recently de-inlined timer. And now that we have C++11 atomics, it may be that we have effectively accomplished this for many consumers. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Tyler Brock [ 11/Jun/14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think we can resolve this ticket as within the CXX scope as the only two files left are the following:
However, this is still unresolved in the server, perhaps this should have been a copy rather than a move so it gets resolved in both places? Would be interested to see what acm thinks. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 07/Apr/14 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Kicking this out to 'unsure' because it isn't clear that this is achievable without significant restructuring. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Tad Marshall [ 14/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
We could also try to minimize our usage of well known Windows types like DWORD (same as uint32_t), keep code out of headers, etc., but I think there are cases where it will be very clumsy to avoid it entirely. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andy Schwerin [ 14/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
tad, milkie, I think that maybe Windows typedefs and #defines should be explicitly excepted from IWYU, in favor of the "include basic.h first in your cpp file" rule. That would get us out of including basic.h/windows_basic.h in header files safely. We should discuss. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Eric Milkie [ 14/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Including windows_basic.h in headers should be allowed, but not for C++ driver headers. We should be very careful about where windows_basic.h is included. Once we have a better C++ driver interface (one that properly adorns its functions with calling-standard macros), it should make it a lot easier to identify which headers are client-facing. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Tad Marshall [ 14/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
So one upshot of this is that header files that use Windows #defines or typedefs will delegate responsibility for their definition to the source file that includes them; correct? This will be an explicit exception to "include what you use". | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Eric Milkie [ 14/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Should we change the Style Guide to mention that we should include basic.h as the first include for every cpp file, and to not include pch.h? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andy Schwerin [ 13/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Header file fix-it day? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Andy Schwerin [ 13/Mar/13 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Problem headers are pch.h, basic.h and windows_basic.h. None of those should be included in other header files without prior authorization. All cpp files should have "mongo/platform/basic.h" as their very first include.
|