Move libultra functions and variables to libultra headers #1468

Thar0 · 2022-12-18T18:44:22Z

Not totally happy with this, since macros.h and the new stack.h are not libultra headers but are still used in some libultra files for now, however this is the bulk of the changes and further improvements will inevitably be found.

…onger use global.h

…a-headers

Dragorn421 · 2022-12-19T02:01:37Z

Not sure where to start reviewing but I noticed FLT_MAX -> MAXFLOAT, the former being C-standard (I think), the latter coming from where?

Thar0 · 2022-12-19T02:12:33Z

In the three MIPS cross compilers I have installed (mips-linux-gnu-, well-known, mips64-ultra-elf-, glank's toolchain, mips64-elf-, libdragon toolchain) I cannot find the definition FLT_MAX, only MAXFLOAT in math.h.

Supposedly FLT_MAX is in float.h, an ISO C99 header, however I can't find a physical copy of such a header (I'd love to know where it is)
It seems MAXFLOAT is not strictly ISO C, it's an "extension", but it seems sufficiently widely adopted?

Dragorn421 · 2022-12-19T13:25:21Z

Makefile

+INC        := -Iinclude -Iinclude/libc -Isrc -Ibuild -I.
+build/src/libultra/%.o: INC := -Iinclude/libc -Iinclude/ultra64 -Isrc/libultra


I can't say I'm a fan of the include path not being the same throughout the repo

I think it's just more correct than using ultra64/... paths in libultra files, and libultra files ideally shouldn't see any headers from the base include dir (although unfortunately as I mentioned I still couldn't get rid of two headers yet)
It may be better if we moved src/libultra to say lib/libultra with it's own makefile or something, but that's a bit too far of a departure from what I wanted to achieve for now (but I think doing this later may be good)

Yeah I understand the appeal when looking at libultra specifically/separately and I agree libultra should be completely cut off from the rest, my beef is mostly/only that it's confusing when navigating the repo

It would be nice to document this somewhere other than "self-documenting" in the Makefile. I wouldn't be opposed to a README file in src/libultra and include/libultra but idk if that would be a popular suggestion

Also I'm starting with criticism but I should also congratulate you on a job well done, it's nice to see header cleanup and this looks quite involved

Dragorn421 · 2022-12-19T13:33:52Z

src/libultra/os/sethwintrroutine.c

@@ -1,11 +1,10 @@
-#include "global.h"
-#include "ultra64/internal.h"
+#include "osint.h"


Suggested change

#include "osint.h"

#include "internal.h"

(This is the least-stuff-inside meaningful header, right?)

Dragorn421 · 2022-12-19T13:42:10Z

include/ultra64/viint.h

@@ -34,4 +37,12 @@
 #define VINTR(v) v
 #define HSTART START


Should the OSViMode related macros be in vi.h instead ?

I'm hesitant to do this since we know what should be in vi.h (os_vi.h), but we don't have a reference viint.h so we can put just about whatever we like in there.

Forgetting about the original headers, I think it makes more sense for them to be with the OSViMode definition,
but I'll leave it up to you and the "follow the original headers" stance

Dragorn421 · 2022-12-19T13:49:39Z

src/libultra/gu/coss.c

@@ -1,4 +1,4 @@
-#include "global.h"
+#include "guint.h"


Suggested change

#include "guint.h"

#include "gu.h"

Note I don't know what you're going for with includes

I assume they should be minimal?
But for example would a

#include "guint.h"

->

#include "ultratypes.h" #include "gu.h"

be better if something didn't need what guint.h itself contains?

I've mostly been using the *int headers for each libultra module, since I believe that's how it was originally based on mdebug includes, but maybe a more minimal approach is just better idk. Though note that as I mention below the minimal set of includes must also contain the prototype for the functions in the file, not just the necessary external declarations that the function(s) uses.

Dragorn421 · 2022-12-19T13:52:04Z

src/libultra/io/dpsetstat.c

@@ -1,4 +1,5 @@
-#include "global.h"
+#include "rcp.h"
+#include "rdp.h"


Suggested change

#include "rdp.h"

?
and dpgetstat

rdp.h contains the prototype for osDpSetStatus, the header containing the prototype needs to be included so the compiler can warn/error if the prototype does not match the function itself

Dragorn421 · 2022-12-25T15:31:32Z

Very interesting last set of changes!
What is the future you envision for explicit-width "ultratypes" s32 & co ? What should use int & co "natives" instead, or not? (does that make sense)

Thar0 · 2022-12-25T17:14:28Z

So far I've only been removing the ultratypes from libc files/functions, since libc shouldn't know anything about libultra. Otherwise I have no current plans to start removing them elsewhere

hensldm · 2022-12-26T18:32:17Z

include/ultra64/cache.h

+#define	OS_DCACHE_ROUNDUP_ADDR(x) (void*)(((((u32)(x) + 0xf) / 0x10) * 0x10))
+#define	OS_DCACHE_ROUNDUP_SIZE(x)   (u32)(((((u32)(x) + 0xf) / 0x10) * 0x10))


Suggested change

#define OS_DCACHE_ROUNDUP_ADDR(x) (void*)(((((u32)(x) + 0xf) / 0x10) * 0x10))

#define OS_DCACHE_ROUNDUP_SIZE(x) (u32)(((((u32)(x) + 0xf) / 0x10) * 0x10))

#define OS_DCACHE_ROUNDUP_ADDR(x) (void*)(((((u32)(x) + 0xF) / 0x10) * 0x10))

#define OS_DCACHE_ROUNDUP_SIZE(x) (u32)(((((u32)(x) + 0xF) / 0x10) * 0x10))

hensldm · 2022-12-26T18:36:33Z

include/ultra64/ai.h

+u32 osAiGetLength(void);
+s32 osAiSetFrequency(u32);
+s32 osAiSetNextBuffer(void*, u32);


Curious was it deliberate to leave out the arg names here, and just use types?

hensldm · 2022-12-26T18:37:35Z

include/ultra64/gu.h

 #define GU_PI 3.1415926

 #define ROUND(x) (s32)(((x) >= 0.0) ? ((x) + 0.5) : ((x) - 0.5))

+void guMtxIdent(Mtx*);


Relating to the arg question again, some functions here have arg names and some don't. Any particular reason why?

Dragorn421 · 2023-01-05T13:05:54Z

include/libc/assert.h

+
+// Static/compile-time assertions
+
+#if defined(__GNUC__) || (__STDC_VERSION__ >= 201112L)


Wouldn't just

Suggested change

#if defined(__GNUC__) || (__STDC_VERSION__ >= 201112L)

#if (__STDC_VERSION__ >= 201112L)

be enough / make more sense? (unless gcc did have the macro way before C11)

Dragorn421 · 2023-01-05T13:07:59Z

include/is_debug.h

+#ifndef ISVIEWER_H
+#define ISVIEWER_H
+
+#include "ultra64.h"


Should this include be in the .c only? It's not needed by the .h, I think

Dragorn421 · 2023-01-05T13:48:52Z

src/libultra/os/exceptasm.s

    // Load cart callback set by __osSetHWIntrRoutine
    lui     $t1, %hi(__osHwIntTable)
    addiu   $t1, %lo(__osHwIntTable)
-    lw      $t2, (OS_INTR_CART*HWINT_SIZE+HWINT_CALLBACK)($t1)
+    lw      $t2, (OS_INTR_CART*HWINT_SIZE+HWINT_HANDLER)($t1)


since HWINT_CALLBACK -> HWINT_HANDLER , should references to "callback" in sentences be changed too?

Dragorn421 · 2023-01-05T13:52:01Z

src/libultra/os/afterprenmi.c

-#include "global.h"
+#include "internal.h"

 s32 osAfterPreNMI(void) {


So per the comment about includes #1468 (comment)
should this include system.h ? (with the osAfterPreNMI prototype)

The same kind of comment seems to apply to other files

AngheloAlf · 2023-11-20T14:44:18Z

include/ultra64/libc.h

+void bzero(void* __s, size_t __n);
+int bcmp(const void* __sl, const void* __s2, size_t __n);
+void bcopy(const void* __src, void* __dest, size_t __n);


The original libultra headers use int instead of size_t. Why deviate from the original headers here?
Also, why the __ prefix on the arguments?

IIRC int is required for matching anyway. Currently in HEAD it's

#ifdef __GNUC__ void bzero(void* __s, unsigned int __n); int bcmp(const void* __sl, const void* __s2, unsigned int __n); void bcopy(const void* __src, void* __dest, unsigned int __n); #else void bzero(void* __s, int __n); int bcmp(const void* __sl, const void* __s2, int __n); void bcopy(const void* __src, void* __dest, int __n); #endif

ariahiro64 · 2024-03-06T05:49:37Z

idk what this entails but theres also the "modern n64 sdk" which includes the entirety of libultra. the legality is murky at best.

Dragorn421 · 2024-09-11T12:39:00Z

We'll eventually need at least part of this PR revived for header cleanup.

The goals with libultra should be clarified: align with ultralib? if not, which includes in .c files (IWYU ?) ?

Thar0 added 4 commits December 18, 2022 03:07

Move libultra functions and variables to headers, libultra files no l…

16328f5

…onger use global.h

Merge branch 'master' of https://github.com/zeldaret/oot into libultr…

32e2123

…a-headers

Remove extern on some function prototypes

740a89c

More libc header changes

e545de1

Dragorn421 added Needs contributor approval labels Dec 19, 2022

Dragorn421 reviewed Dec 19, 2022

View reviewed changes

Thar0 added 2 commits December 25, 2022 00:40

assert.h, is_debug.h, printing function fixes, more libc fixes

d96b6df

Fix bss reordering

bb0af75

hensldm reviewed Dec 26, 2022

View reviewed changes

fig02 added the Waiting for author There are requested changes that have not been addressed label Jan 2, 2023

Dragorn421 reviewed Jan 5, 2023

View reviewed changes

hensldm mentioned this pull request Feb 27, 2023

Minor Misc Cleanup 3 #1502

Merged

fig02 removed Needs contributor approval labels Aug 27, 2023

Thar0 mentioned this pull request Oct 23, 2023

libc cleanup #1568

Merged

AngheloAlf reviewed Nov 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move libultra functions and variables to libultra headers #1468

Move libultra functions and variables to libultra headers #1468

Thar0 commented Dec 18, 2022

Dragorn421 commented Dec 19, 2022

Thar0 commented Dec 19, 2022

Dragorn421 Dec 19, 2022

Thar0 Dec 19, 2022

Dragorn421 Dec 19, 2022

Dragorn421 Dec 19, 2022

Dragorn421 Dec 19, 2022

Dragorn421 Dec 19, 2022

Thar0 Dec 19, 2022

Dragorn421 Dec 19, 2022

Dragorn421 Dec 19, 2022

Thar0 Dec 19, 2022

Dragorn421 Dec 19, 2022

Thar0 Dec 19, 2022

Dragorn421 commented Dec 25, 2022

Thar0 commented Dec 25, 2022

hensldm Dec 26, 2022

hensldm Dec 26, 2022

hensldm Dec 26, 2022

Dragorn421 Jan 5, 2023

Dragorn421 Jan 5, 2023

Dragorn421 Jan 5, 2023

Dragorn421 Jan 5, 2023

AngheloAlf Nov 20, 2023

cadmic Aug 14, 2024 •

edited

Loading

ariahiro64 commented Mar 6, 2024

Dragorn421 commented Sep 11, 2024

		INC := -Iinclude -Iinclude/libc -Isrc -Ibuild -I.
		build/src/libultra/%.o: INC := -Iinclude/libc -Iinclude/ultra64 -Isrc/libultra

		#define OS_DCACHE_ROUNDUP_ADDR(x) (void)(((((u32)(x) + 0xf) / 0x10) 0x10))
		#define OS_DCACHE_ROUNDUP_SIZE(x) (u32)(((((u32)(x) + 0xf) / 0x10) * 0x10))


		// Static/compile-time assertions

		#if defined(__GNUC__) \|\| (__STDC_VERSION__ >= 201112L)

	#if defined(__GNUC__) \|\| (__STDC_VERSION__ >= 201112L)
	#if (__STDC_VERSION__ >= 201112L)

Move libultra functions and variables to libultra headers #1468

Are you sure you want to change the base?

Move libultra functions and variables to libultra headers #1468

Conversation

Thar0 commented Dec 18, 2022

Dragorn421 commented Dec 19, 2022

Thar0 commented Dec 19, 2022

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

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

Dragorn421 commented Dec 25, 2022

Thar0 commented Dec 25, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadmic Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

ariahiro64 commented Mar 6, 2024

Dragorn421 commented Sep 11, 2024

cadmic Aug 14, 2024 •

edited

Loading