Skip to content

Conversation

@j-jorge
Copy link
Contributor

@j-jorge j-jorge commented Jul 10, 2023

Hello, this PR contains many small changes to allow unity builds (a.k.a jumbo builds) on a subset of the sources:

  • avoid "using namespace" at file scope (e.g. USING_NAMESPACE_AX),
  • #undef our file-specific #defines (e.g #define LOG_TAG … in audio-related files),
  • and avoid having static functions with identical name and signature (e.g. readBEUint16 in both etc1.cpp and etc2.cpp).

For context, for my project I compile a subset of axmol of 180 files. Using the default build it takes 3m50s on my laptop (13m30s user time), but if I use unity builds it's only 1m50s (5m10s user time).

@rh101
Copy link
Contributor

rh101 commented Jul 10, 2023

Shouldn't all those CPP files, like PolygonInfo.cpp, have NS_AX_BEGIN and NS_AX_END tags at the start and end of the file respectively, since all of the code is part of the ax namespace? The real problem is that USING_NS_AX; should never have been used in those files.

That is the convention throughout the engine code, and it's more readable too, so if there is no specific reason to be adding the namespace to each function etc, then please use NS_AX_BEGIN and NS_AX_END tags instead.

@j-jorge
Copy link
Contributor Author

j-jorge commented Jul 10, 2023

NS_AX_BEGIN and NS_AX_END would work too, it's just that with this convention it is hard to tell in which namespace belongs the function we are looking at, while having the ax:: prefix makes it clear. For example I doubt the static inline uint8_t clamp(int x) at line 2609 of Image.cpp is supposed to be part of the ax namespace. There are so many weird patterns with this usage, e.g., same file, getDevicePVRPixelFormat is in the ax namespace while makeFourCC is in an anonymous namespace (still in namespace ax though). So maybe we don't use the same metric to measure readability ;)

Anyway, if it is a convention, I can switch to the NS_AX_BEGIN/NS_AX_END pair.

@rh101
Copy link
Contributor

rh101 commented Jul 10, 2023

There are so many weird patterns with this usage, e.g., same file, getDevicePVRPixelFormat is in the ax namespace while makeFourCC is in an anonymous namespace (still in namespace ax though)

I never actually noticed that, but the anonymous namespace is used within the ax namespace, and it may be related to ADL, with more info here? (Just a wild guess... I honestly wouldn't know why it was done that way).

So maybe we don't use the same metric to measure readability ;)

Fair point!

For example I doubt the static inline uint8_t clamp(int x) at line 2609 of Image.cpp is supposed to be part of the ax namespace.

static inline uint8_t clamp(int x) in Image.cpp is something I think I added years ago, and at the time I didn't think anything of putting it in an anonymous namespace. If it's better to change it to be the same as other static method implementations, then please do so.

@j-jorge j-jorge force-pushed the fix-unity-build branch 4 times, most recently from a8b2611 to ac3104f Compare July 14, 2023 19:50
@halx99
Copy link
Collaborator

halx99 commented Jul 15, 2023

well, you merge early feture (not complete yet) to your branch, due to this PR is minor improvements, could you recreate another PR based on main branch?
refer to: #1271

In order to allow unity builds we need to avoiding "using namespace"
at file scope, #undef our file-specific #defines, and avoid having
static functions with identical name and signature.
@j-jorge
Copy link
Contributor Author

j-jorge commented Jul 15, 2023

Ah! It explains all these commits suddenly popping in the PR :) Thanks for pointing that out.

I made the changes discussed about USING_NS_AX and NS_AX_BEGIN. The scope of functions like clamp is left unmodified to avoid putting too many things in the PR.

@halx99
Copy link
Collaborator

halx99 commented Jul 15, 2023

lgtm

@halx99 halx99 merged commit a894005 into axmolengine:main Jul 15, 2023
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.

3 participants