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

Change member variable from 'is' to 'is_' to avoid possible (macro) name collision #4406

Closed
wants to merge 2 commits into from

Conversation

LiAuTraver
Copy link

@LiAuTraver LiAuTraver commented Jun 26, 2024

TL; DR: This pull request was attempting to addresses potential naming collisions with macros defined by users of the library. Specifically, the member variable is in the input_adapter class has been renamed to is_ and sb to sb_.


Description

we encountered issues caused by some users who came from Java/C# and defined is as a macro in their code. This macro interfered with the library internal implementation, resulting in unexpected behavior and compilation errors due to the macro replacing variables named is in the library's internal implementation.

I admit defining is is very inappropriate and was a bad idea and inadvisable for them. It's the user's fault to do that and definitely out of control of this json library, however, I believe that issues caused by such keyword-like named variables are avoidable. To prevent potential naming collisions and improve compatibility with user-defined macros, in the input_adapter class, I

  • Renamed the member variable is to is_.
  • Renamed the member variable sb to sb_ (naming consistency).

Pull request checklist

  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

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.

1 participant