Skip to content

winldap: Fix ldap_get_dn function prototype - #2222

Open
Jay Satiro (jay) wants to merge 2 commits into
MicrosoftDocs:docsfrom
jay:ldap_get_dn
Open

Jay Satiro (jay) wants to merge 2 commits into
MicrosoftDocs:docsfrom
jay:ldap_get_dn

Conversation

@jay

Copy link
Copy Markdown
Contributor
  • 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


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.

- 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
@prmerger-automator

Copy link
Copy Markdown

Jay Satiro (@jay) : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — accurate technical correction. Approved.

@jay

Copy link
Copy Markdown
Contributor Author

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.

@GrantMeStrength

Copy link
Copy Markdown
Contributor

Copilot /review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed ✅ — Documentation improvement looks good.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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(...);
#endif

PTCHAR 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, or LDAPModA even though the generic name maps to the Unicode entry point when LDAP_UNICODE != 0.
  • Only ldap_get_option and ldap_set_option have 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.h selects using LDAP_UNICODE (which merely defaults from UNICODE).

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.

@jay

Copy link
Copy Markdown
Contributor Author

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:

The SDK sets LDAP_UNICODE to 1 or 0 depending on whether UNICODE is defined. You may override that behavior by setting it yourself.

I would encourage MS to pursue the "durable solution" but I don't know how to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants