Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ Client Library Public Header Structure #8348

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

antkmsft
Copy link
Member

No description provided.

fantasticgizmos/
fantasticgizmos_models.hpp <-- all models for the Gizmos service.
amazing_gadget_client.hpp
amazing_gadget_client_options.hpp <-- all options for the Amazing Gadget client, both Client Options and operation (method) options.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
amazing_gadget_client_options.hpp <-- all options for the Amazing Gadget client, both Client Options and operation (method) options.
amazing_gadget_client_options.hpp <-- all options for the Amazing Gadget client, both client options and operation (method) options.

@@ -697,6 +697,27 @@ struct HashComputation {
} // unnamed namespace
{% endhighlight %}

#### Client Library Public Header Structure

Example: Azure Fantastic Gizmos service has two clients, AmazingGadgetClient and AwesomeWidgetClient.
Copy link
Member

@ahsonkhan ahsonkhan Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this example is great in illustrating/clarifying the expected physical structure of C++ header files on disk.

That said, we should add a "MUST/DO" guideline with an id above the example to highlight this is what we expect:

Suggested change
Example: Azure Fantastic Gizmos service has two clients, AmazingGadgetClient and AwesomeWidgetClient.
{% include requirement/MUST id="cpp-design-physical-public-headers" %} structure the public header files using separate client specific filenames for clients, options, LROs, and pagers, and service specific filename for models.
Example: Azure Fantastic Gizmos service has two clients, AmazingGadgetClient and AwesomeWidgetClient.

awesome_widget_client_paged_responses.hpp
dll_import_export.hpp <-- AZURE_GIZMOS_DLLEXPORT macro definition.
rtti.hpp <-- AZURE_GIZMOS_RTTI macro definition.
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a note after to highlight the importance of this in C++, since header file names/locations are part of the public contract in a source distributed language like C++, which doesn't show up in other languages.

Suggested change
```
```
**Note:** The names and location of public header files are part of the public API surface in C++ and have to be stable to avoid breaking end customers who `#include` them directly in their applications.

Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for updating the guidelines!

@ahsonkhan
Copy link
Member

We could consider using the opportunity to update tables SDK public headers to align with this guideline before we GA (since we can't change them post-GA without it being a breaking change):
https://github.com/Azure/azure-sdk-for-cpp/blob/e31710264715cd611f04ae695e36067823dd64f1/sdk/tables/azure-data-tables/inc/azure/data/tables/models.hpp#L1

Rename: models.hpp -> tables_models.hpp
Move TableClient and TableServiceClient specific options and paged response types into their own file:

  • table_client_options.hpp / table_service_client_options.hpp
  • QueryTables paged response on TableServiceClient: table_service_client_paged_responses.hpp
  • QueryEntity paged response on TableClient: table_client_paged_responses.hpp

There are no LROs in Tables as far as I can tell, so no change needed around that.

For existing GA'd services, we'd leave them as-is.

cc @gearama

@ahsonkhan
Copy link
Member

FYI @ronniegeraghty as the C++ PM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants