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

Unify lengthof/string_lengthof -like macros #61537

Merged
merged 12 commits into from
Nov 24, 2021
Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Nov 12, 2021

  • Move src/native/common to src/native/minipal
  • Define _countof and _strcountof.
  • Replace all _countof, lengthof, NumItems, ARRAY{_}SIZE like macros with _countof.
  • Replace all lengthof and StrLen like macros with _strcountof.
    • Also replaced _countof() - 1 patterns to _strcountof() in few places.

In second commit, ran editor macro to cleanup trailing whitespaces in files touched by the first commit.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 12, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@AraHaan
Copy link
Member

AraHaan commented Nov 13, 2021

What are the MENIPAL_* macros used here defined as?

@am11 am11 force-pushed the feature/minipal branch 8 times, most recently from eb0d614 to 1725ec7 Compare November 13, 2021 04:29
@am11
Copy link
Member Author

am11 commented Nov 13, 2021

What are the MENIPAL_* macros used here defined as?

There were about 32 definitions of those macros repo-wide, this patch moves the definitions in a single place:

#define MINIPAL_LENGTHOF(arr) (sizeof(arr)/sizeof(arr[0]))
// Number of characters in a string literal. Excludes terminating NULL.
#define MINIPAL_STRLEN(str) (MINIPAL_LENGTHOF(str) - 1)

@jkotas
Copy link
Member

jkotas commented Nov 13, 2021

What is the most common name used for this macro in the universe? Would it look better to standardize on that name without any prefix?

@vargaz
Copy link
Contributor

vargaz commented Nov 13, 2021

There is no common name i think, mono uses the name from glib.

@SingleAccretion
Copy link
Contributor

Note that the Jit also uses an ArrLen function for this. I will say that if it were for me to decide, I'd replace all the uses of _countof in the Jit with it, but, heh, it is not :).

@am11
Copy link
Member Author

am11 commented Nov 13, 2021

I don't have strong opinions about the name (naming stuff is hard), so whatever is decided I'm fine with it; now that find & replace after this patch is much easier. ;)

Prefix convention (used heavily in mono) helps identifying where the macro or function definitions are coming from. Recently, we prefixed functions with minipal_. I followed the same here for macros to stay consistent that way.

@am11
Copy link
Member Author

am11 commented Nov 22, 2021

Renamed MINIPAL_{LENGTHOF, STRLEN} to _countof and _strcountof respectively.
CI test failures are unrelated to changes.

@vargaz
Copy link
Contributor

vargaz commented Nov 22, 2021

That doesn't match the mono coding conventions and looks very much like some kind of compiler intrinsic.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2021

@vargaz What would be your preferred name?

@vargaz
Copy link
Contributor

vargaz commented Nov 22, 2021

Not sure this particular macro needs to be unified between the runtimes, both of them already has their own version.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2021

I hope that we will share more code between the runtimes over time. I think it is important that we get the details like these unified to something reasonable. Do you have an opinion what should be the name to standardize on in the code shared between runtimes for this one?

@vargaz
Copy link
Contributor

vargaz commented Nov 22, 2021

Imho the problem is that the runtimes have different coding conventions, so not sure a macro name can fit both equally well. I'd prefer the traditional uppercase macro naming style.

@am11
Copy link
Member Author

am11 commented Nov 22, 2021

src/native/eventpipe is another example of code shared between runtimes, where we have function, as well as the macro names prefixed and macros are upper cased. coreclr uses many different conventions (prefix, no prefix, upper, lower, mixed casing) for macros so there is no problem in changing it to whichever name is suitable.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2021

I'd prefer the traditional uppercase macro naming style.

Would ARRAY_SIZE work?

@jkotas
Copy link
Member

jkotas commented Nov 23, 2021

Based on the feedback, here is my proposal to land this PR:

  • Rename _countof/_strcountof to ARRAY_SIZE and STRING_LENGTH. I think it is reasonable convention to have macros in all caps and I agree that _countof looks more like a compiler intrinsic.
  • Keep using G_N_ELEMENTS in mono since it is used fairly consistently there and follows the Mono coding style.

@am11 If there is no push back in next two days, could you please implement these change? I am sorry about the back and forth, and thank you for your work on various cleanups!

@vargaz
Copy link
Contributor

vargaz commented Nov 24, 2021

Sounds good

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants