Memstuff #1164

EllipticEllipsis · 2022-02-28T19:31:38Z

Name all the memory/string.h functions I could find, document them as consistently as possible. OoT has multiple memsets and memcpys, some of which are rather non-conformant, so I added fairly complete docs for them. It's not clear how to name the ones in code_80069420

I did not document the libultra string.h since it will likely be reworked in a significant way in future anyway.

Also added some more sizeofs to places that were missing them. I didn't touch z_sram since the whole thing needs a rework anyway, or z_demo, because I'm not sure which thing is appropriate to take the sizeof in that function. The most significant change was in BgCobra: given that this has a texture in its struct that it has to align, it has an oversized buffer, so I added some appropriate defines to be able to do this. There is a remaining question of whether to do

Lib_MemSet(shadowTex, sizeof(this->shadowTextureBuffer) & ~0xF, 0)

or

Lib_MemSet(shadowTex, COBRA_SHADOW_TEX_SIZE, 0);

Drafting until #1162 is merged... apparently I started this a while ago and documented fmodf too.

Dragorn421 · 2022-02-28T21:38:30Z

include/functions.h

@@ -831,7 +831,7 @@ void func_800645A0(GlobalContext* globalCtx, CutsceneContext* csCtx);
 void Cutscene_HandleEntranceTriggers(GlobalContext* globalCtx);
 void Cutscene_HandleConditionalTriggers(GlobalContext* globalCtx);
 void Cutscene_SetSegment(GlobalContext* globalCtx, void* segment);
-void* MemCopy(void* dest, void* src, s32 size);
+void* MemCpy(void* dest, const void* src, s32 size);


since with the capitalization it's already not memcpy, is there really a point to removing the o
This may just bring more confusion between memcpy and MemCpy, not that the o would help much but it's still something (at least both functions are conformant memcpys, but one is in code so may not be used in boot so it's not entirely irrelevant to prevent confusion)

Ideally these get a proper prefix anyway. Unfortunately there's very little context for how they're used: it's essentially "the one used outside boot", and the MemSet in here isn't used at all. It's totally gone in MM, and its duties reassigned to bcopy or __osMemcpy, so practically the advice should just be "don't bother using this one".

Dragorn421 · 2022-03-02T22:55:08Z

src/code/code_80069420.c

+ *
+ * @param[in,out] dest address to start at
+ * @param[in] val value to write (s32, but interpreted as u8)
+ * @param[in] len number of bytes to write. Has to be s32: it is not possible to set more than 2 GB of RAM at once.


I'm not sure the comment about 2GB of ram is relevant
Or at least it's not more relevant here than anything (a lot of things) that depend on addresses fitting in a s32

Mm, I suppose it would probably be better to note that this cannot sensibly be a size_t (the function argument can, but the temp must be s32, and it feels disingenuous to allow the user to accidentally feed it a large integer that gets recast into a small negative one).

Dragorn421 · 2022-03-02T22:59:10Z

src/code/__osMemset.c

+ *
+ * There are two other memsets in this codebase,
+ * @sa Lib_MemSet(), MemSet()
+ * This one is used in __osMalloc, z_quake, z_view, and z_camera


Is there a point to listing where a function is used?
it's only a grep/ctrl+shift+f away to find that info,
and these names may also change eventually so it's going to get outdated too

This is probably a habit from looking on GitHub with its useless searching. I think it's useful to explicitly note patterns of usage (it makes them easier to compare when you have them explicitly written down, for example), but I can see that people would regard this an unnecessary in "final" documentation.

I agree with dragorn that its not worth commenting on where it gets used

Dragorn421 · 2022-03-02T23:02:34Z

src/overlays/actors/ovl_Bg_Jya_Cobra/z_bg_jya_cobra.c

-    temp_s2 = ALIGN16((s32)(&this->shadowTexture));
-    Lib_MemSet(temp_s2, 0x1000, 0);
+    shadowTex = COBRA_SHADOW_TEX_PTR(this);
+    Lib_MemSet(shadowTex, sizeof(this->shadowTextureBuffer) & ~0xF, 0);


This should indeed use COBRA_SHADOW_TEX_SIZE imo, these zeros are meant as the default color (transparent black) afaict, it wouldn't care if the buffer happened to be oversized, if that's what using sizeof here is meant for

The point of this is to allow downsizing the buffer to the appropriate size, but it probably does make more sense to use the texture size directly. I'll change it.

Roman971

Sorry for the very late review on this one.
The changes look pretty good to me, and I just have some comments on the documentation style

Roman971 · 2022-04-30T17:31:01Z

src/code/__osMemset.c

+/**
+ * @brief memset: sets @p len bytes to @p val starting at address @p dest .
+ *
+ * There are two other memsets in this codebase,
+ * @sa Lib_MemSet(), MemSet()
+ *
+ * @param[in,out] dest address to start at
+ * @param[in] val value to write (s32, but interpreted as u8)
+ * @param[in] len number of bytes to write
+ *
+ * @return dest
+ */


The documentation here looks good but I have a few comments regarding the syntax (based on our doxygen guide and what we've done so far):

We generally don't use @brief, and I believe the result is the same without it when it's the first line, so we can probably just omit it

We generally don't use [in]/[in,out] either for @param, and they don't seem very useful when combined with proper descriptions for each param, so I would avoid them to keep things simple

It looks like @see can be used instead of @sa and sounds much clearer imo

I believe backticks (``) can be used instead of @p, which might help with readability when looking directly at the comment in code

Note: My knowledge on doxgen is fairly limited so a lot of this could be wrong, but I wanted to at least bring it up since I don't think we've used or discussed any of these keywords in the past

src/code/fmodf.c

@brief

* Un-fake a couple of matches in memory manip functions * Document fmodf * Un-fake a couple of matches in memory manip functions * Document fmodf * Rename functions and files * Document memmove, memsets, memcpys * Format * Sort out some missing sizeofs * Name fmodf * Rename local variables * size_t * Use COBRA_SHADOW_TEX_SIZE * Review * Tweak the Doxyfile to remove @brief requirement * Roman's review * Fix a bug comment * Change fmodf

EllipticEllipsis added 9 commits January 17, 2022 00:28

Un-fake a couple of matches in memory manip functions

d389eda

Document fmodf

14c1ea0

Un-fake a couple of matches in memory manip functions

ddd272e

Document fmodf

c4f3126

Rename functions and files

71c356f

Document memmove, memsets, memcpys

b1dcb3b

Format

c160a55

Sort out some missing sizeofs

97f7dad

Name fmodf

e239738

Dragorn421 reviewed Feb 28, 2022

View reviewed changes

EllipticEllipsis added 2 commits March 1, 2022 19:34

Rename local variables

46d2c8c

Merge remote-tracking branch 'upstream/master' into memstuff

266ea99

EllipticEllipsis marked this pull request as ready for review March 1, 2022 19:36

size_t

6101e8c

Dragorn421 reviewed Mar 2, 2022

View reviewed changes

EllipticEllipsis added 2 commits March 6, 2022 05:48

Use COBRA_SHADOW_TEX_SIZE

4a78ba0

Review

05cdded

Dragorn421 approved these changes Mar 10, 2022

View reviewed changes

fig02 approved these changes Mar 13, 2022

View reviewed changes

EllipticEllipsis added 2 commits April 18, 2022 18:39

Merge remote-tracking branch 'upstream/master' into memstuff

3a78e96

Merge remote-tracking branch 'upstream/master' into memstuff

b73d8e2

Roman971 reviewed Apr 30, 2022

View reviewed changes

EllipticEllipsis added 5 commits April 30, 2022 22:17

Merge branch 'memstuff' of github.com:EllipticEllipsis/oot into memstuff

4119d5c

Merge branch 'memstuff' of github.com:EllipticEllipsis/oot into memstuff

005ae65

Tweak the Doxyfile to remove @brief requirement

25a21a2

Roman's review

9234dc4

Fix a bug comment

a2f952b

fig02 approved these changes May 1, 2022

View reviewed changes

Dragorn421 approved these changes May 1, 2022

View reviewed changes

Roman971 reviewed May 1, 2022

View reviewed changes

src/code/fmodf.c Outdated Show resolved Hide resolved

Change fmodf

a2b3ff9

fig02 approved these changes May 1, 2022

View reviewed changes

Roman971 approved these changes May 1, 2022

View reviewed changes

Roman971 merged commit e84f5ab into zeldaret:master May 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memstuff #1164

Memstuff #1164

EllipticEllipsis commented Feb 28, 2022

Dragorn421 Feb 28, 2022

EllipticEllipsis Mar 1, 2022

Dragorn421 Mar 2, 2022

EllipticEllipsis Mar 2, 2022 •

edited

Loading

Dragorn421 Mar 2, 2022

EllipticEllipsis Mar 2, 2022

fig02 Mar 7, 2022

Dragorn421 Mar 2, 2022

EllipticEllipsis Mar 2, 2022

Roman971 left a comment

Roman971 Apr 30, 2022 •

edited

Loading

Memstuff #1164

Memstuff #1164

Conversation

EllipticEllipsis commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EllipticEllipsis Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roman971 left a comment

Choose a reason for hiding this comment

Roman971 Apr 30, 2022 • edited Loading

Choose a reason for hiding this comment

EllipticEllipsis Mar 2, 2022 •

edited

Loading

Roman971 Apr 30, 2022 •

edited

Loading