winldap: Fix ldap_get_dn function prototype - #2222
Jay Satiro (jay) wants to merge 2 commits into
Conversation
- Override the syntax manually to declare ldap_get_dn returns PTCHAR. Prior to this change the auto-generated syntax erroneously said that ldap_get_dn returned PCHAR instead of PTCHAR. The bad syntax probably has to do with the way the unusual way ldap functions are declared in winldap.h. ldap_get_dn is either a macro that maps to ldap_get_dnW (LDAP_UNICODE defined, returns PWCHAR) or given its own protoype the same as ldap_get_dnA (no LDAP_UNICODE, returns PCHAR). Closes #xxxx
|
Jay Satiro (@jay) : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change. |
John Kennedy (GrantMeStrength)
left a comment
There was a problem hiding this comment.
LGTM — accurate technical correction. Approved.
|
Please note many ldap functions are affected by this and it may be better for MS to make some structural change to address it due to the unusual way the functions are declared, as mentioned in my original post. |
|
Copilot /review |
John Kennedy (GrantMeStrength)
left a comment
There was a problem hiding this comment.
Reviewed ✅ — Documentation improvement looks good.
There was a problem hiding this comment.
🟡 Changes recommended
PTCHAR does not reflect configurations where LDAP_UNICODE differs from UNICODE.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a manual ldap_get_dn prototype to correct generated documentation.
Changes:
- Documents an encoding-neutral return type as
PTCHAR.
File summaries
| File | Description |
|---|---|
sdk-api-src/content/winldap/nf-winldap-ldap_get_dn.md |
Adds the manual function syntax. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ## -syntax | ||
|
|
||
| ```cpp | ||
| WINLDAPAPI PTCHAR LDAPAPI ldap_get_dn( |
John Kennedy (GrantMeStrength)
left a comment
There was a problem hiding this comment.
Maintainer audit: systemic WinLDAP documentation issue
Requesting changes because the proposed PTCHAR override is not correct for every supported preprocessor configuration. This supersedes my earlier approvals.
winldap.h selects the generic API with LDAP_UNICODE, not directly with UNICODE:
#ifndef LDAP_UNICODE
#ifdef UNICODE
#define LDAP_UNICODE 1
#else
#define LDAP_UNICODE 0
#endif
#endif
#if LDAP_UNICODE
#define ldap_get_dn ldap_get_dnW // returns PWCHAR
#else
WINLDAPAPI PCHAR LDAPAPI ldap_get_dn(...);
#endifPTCHAR follows UNICODE, so it can disagree with the function selected when a caller defines LDAP_UNICODE independently. The generic return type is conditionally PWCHAR or PCHAR; there is no existing TCHAR typedef governed by LDAP_UNICODE.
Scope confirmed by repository/header audit
- 72 generic WinLDAP names are controlled by
LDAP_UNICODE. - 62 have generic documentation topics in this repository.
- 60 of those topics expose character-sensitive ANSI types such as
PSTR,PCHAR,PZPSTR,LDAPControlA, orLDAPModAeven though the generic name maps to the Unicode entry point whenLDAP_UNICODE != 0. - Only
ldap_get_optionandldap_set_optionhave encoding-neutral visible prototypes. - The affected topics span connection, bind, search, modify, rename, add, compare, delete, result/control parsing, attribute/value/DN, sort, paging, reference, and extended-operation APIs.
- The generated A/W-topic warning also says selection is based on
UNICODE; strictly,winldap.hselects usingLDAP_UNICODE(which merely defaults fromUNICODE).
This confirms the PR author's observation that the problem is systemic and caused by the unusual conditional declarations in winldap.h. I found no other open fix for the underlying harvesting behavior.
Required direction
Please do not merge this as a standalone PTCHAR substitution. At minimum, this topic must document both branches explicitly (PWCHAR when LDAP_UNICODE != 0, PCHAR otherwise) and explain the override behavior. The durable solution should address the shared harvesting/template behavior for the affected WinLDAP generic topics rather than applying approximately 60 independent PTCHAR substitutions.
|
I wasn't sure the best way to explain, but I took a shot as you can see in the fixup commit I just pushed:
I would encourage MS to pursue the "durable solution" but I don't know how to do that. |
Prior to this change the auto-generated syntax erroneously said that ldap_get_dn returned PCHAR instead of PTCHAR.
The bad syntax probably has to do with the way the unusual way ldap functions are declared in winldap.h. ldap_get_dn is either a macro that maps to ldap_get_dnW (LDAP_UNICODE defined, returns PWCHAR) or given its own protoype the same as ldap_get_dnA (no LDAP_UNICODE, returns PCHAR).
Closes #xxxx
AFAICT, LDAP_UNICODE is only used internally and set depending on UNICODE. They are different symbols so LDAP_UNICODE can be different from UNICODE. This presents a dilemma because LDAP_UNICODE does not have specific dependent types like TCHAR. It may be more correct (but also confusing) to add in remarks that if LDAP_UNICODE is defined to 0 then the return type is PCHAR.
Many ldap functions have incorrect prototype documentation but I only changed one. There is no way to open an issue so I opened a PR with this one change to notify. I suggest MS rather tackle this issue wholesale.