All global-namespaced types ending with _t must be renamed

[mkoscumb]

Issue
The POSIX standard reserves any type identifier ending with underscore-lower-case-t (_t). As the Client Telemetry SDK defines a number of these in the CAPI header (mat.h), this has the potential for collisions and compiler errors if a translation unit includes a POSIX header which uses these types and mat.h

Office has observed these collisions including mat.h in files that also (through a chain of includes) include MACH headers on OSX.

Impact
From a public API perspective, this appears to only impact types in the global namespace, which means the fix can be scoped entirely to mat.h. There will of course be a few unit test and implementation changes as well.

Proposed Solution
Prefix all mat.h types with evt_* (thanks Max for the prior art), and remove all _t suffixes. As type names are changing, this should also cause a major-version-increment. Also update the coding-standards document to add a section for C type names.

[maxgolov]

I would suggest to scope this change mostly to the type that is causing the break, associated with recent changes (changes that enabled HTTP C API support):

• task_t
• http_request_type_t
• http_result_t

From a practical standpoint, it appears that the scope of this change should apply to task_t only .. And while it may or may not be true for POSIX, POSIX being Operating System-level design - would unlikely implement HTTP client as a fundamental primitive.

All other types are already prefixed with evt_ - Why evt? Because for some time we've been swinging back and forth between MAT (telemetry), MAE (events), so evt_ - stands for events, - i.e. most primitives already provide "unique enough" prefix. I'd suggest to avoid refactoring these to minimize the impact on API surface.

task_t and http types are used by MIP SDK --- @trevorlacey-msft f.y.i.

[maxgolov]

evt_task_t ?

[mkoscumb]

evt_task_t ?

That still breaks from the POSIX standard requirement of reserving any types that end with underscore-t, I'm fine with evt_* as the prefix, especially if we use it elsewhere (updated Issue accordingly) - though I think mat.h should be renamed evt.h in that case, I don't think we need to do that as part of this Issue.

I would suggest to scope this change mostly to the type that is causing the break, associated with recent changes (changes that enabled HTTP C API support):

Why? I've fixed the break locally in Office, this is not an issue that requires we take on technical debt for a quick turnaround. Limiting this to only one type of three will only cause confusion in the SDK's naming scheme, which we should be unified on.

[maxgolov]

My suggestion is we tackle this in several stages:

• add evt_ prefix to anything that has _t suffix. Although that is technically breaking POSIX recommendation (or an optional extension to C standard), since we are not an Operating System-level library per se, we do not have to be compliant. From a practical standpoint - we are not going to clash with anything. There is a disclaimer in that requirement that "any conflict is unintentional". So we are not going to intentionally break anything.
• have the discussion about evt.h vs mat.h, and evt_ vs mat_ prefix as part of OSS story. This is something fairly easy to refactor, but since it's a breaking change that might be impacting MIP SDK, for example, I'd rather avoid doing this right now... I agree we'd need to update the SDK version to, say, v3.5 and then rename. Customers then also need to refactor their code accordingly. Frankly, I'd rather avoid it: POSIX compliance is not a requirement for us per se (Windows is not POSIX compliant, Android is not POSIX compliant), and making a reasonable effort to avoid the clash with existing standard POSIX types should be sufficient.

[maxgolov]

Examples of code bases that are NOT following the POSIX guideline, but running fine on POSIX-compliant Operating Systems:

• nginx (heavily using _t suffix in its coding guidelines)
• Android (uses its own status_t - is not a standard POSIX type)
• Windows (several SDK headers contain _t , in fact Windows SDK goes as far as adding define min and define max as macros, which totally breaks C++ std::min and std::max... even worse than having a type that has a _t suffix in it)

[mkoscumb]

EDIT: I stand corrected, Windows does provide an optional POSIX implementation https://en.wikipedia.org/wiki/Windows_Subsystem_for_Linux. That said, windows.h (and subheaders in the SDK) defining _t types is allowed, as it itself is the implementation, and is thus allowed to use things that are reserved for the implementation.

I don't know why we wouldn't just remove the _t all together from our mat.h types. The type name is already changing, there's going to need to be a code change, so might as well get it all in a compliant state.

POSIX compliance (or best effort that we can) should be a requirement for us if we intend to run in POSIX environments, certainly we shouldn't actively defy standards when we have the opportunity to adhere to them, especially when the set of consumers of mat.h is relatively small. Paying the cost now thus is way better than having to pay the cost after going to OSS and having potentially gazillions of diverse and disparate consumers.

Finally, the fact that other projects have done things that violate rules for things reserved by the implementation doesn't give us justification to do so.

[maxgolov]

@mkoscumb - we'd have to be reasonable and practical with respect to who's already consuming the implementation in environments outside of Office. I don't find it practical nor reasonable to refactor this for v3.2. The best "least cost effort" course of action for v3.2 is to add evt_ prefix to task_t , that resolves your build break.

We can debate what's best for v3.5+ , and how we can address this / document this to all customers upgrading from v3.2 to v3.5. It'd be great to provide some markdown document with upgrade instructions, assuming we do not want to #define SOMETHING_t SOMETHING to assist with an upgrade.

I don't think we can afford renaming this type in v3.2 at this point neither in October nor in November timeframe.

[maxgolov]

@mkoscumb - Hi Matt, I looked closer at this issue. There is a practical reason why we can never fully satisfy this requirement. I double-checked that our fav nlohmann/json.hpp library - actually has its own types that are named as value_t.

While POSIX (soft) requirement is nice-to-satisfy, I do not believe the status_t on its own is presenting a challenge to any of our customers. We do run just fine on Android, for example, which also has its own definition of the same name (in a different namespace). There are other types and macros that do cause some conflicts sometimes.. This prefix issue - isn't one of them. At least not immediately, and not for the last N years. Even if we fix that one, we'd still permanently have value_t in json and maybe similarly named _t types elsewhere in some of our deps.. So our combined codebase our code + deps will never truly be able to be entirely compliant anyways.

I think we should just let it go. Proof link showing json.hpp having enum class value_t type:
https://github.com/nlohmann/json/blob/6b74772fe8957ec88efbfb4eed4c7d79d3c0f57a/doc/examples/operator__value_t.cpp
Technically it's also in violation, as it seems, of that (rather soft) POSIX guideline.

[maxgolov]

....MAYBE only a change in mat.h - that I think is a possibility. But not in C++?