Add basic win support #67

Enjection · 2023-01-28T21:44:10Z

No description provided.

pantoniou · 2023-01-31T07:42:11Z

I had some time to look at this PR more closely.

First of all, it is a significant amount of work.
Most of it deals with the unportable (but very convenient) gcc extensions.

Obviously it's still a WIP and could use splitting up in more manageable chunks.

pantoniou · 2023-01-31T07:43:14Z

include/libfyaml.h


 #if defined (__unix__) || (defined (__APPLE__) && defined (__MACH__))
 #include <unistd.h>
+#define FY_ALLOCA(x) alloca(x)


Since we're building internally you could just define an alloca() macros instead of FY_ALLOCA and changing all the call sites...

Indeed. Additionally you'll likely rather want _malloca. According to MS docs, _alloca is deprecated and _malloca apparently has the same signature so it should just work the same as MR _alloca or standard C alloca.

pantoniou · 2023-01-31T07:44:01Z

include/libfyaml.h

 #define FY_NT	((size_t)-1)

 #if defined(__GNUC__) && __GNUC__ >= 4
+#define FY_ATTRIBUTE(attr) __attribute ((attr))


I'd like a separate PR with just the FY_ATTRIBUTE changes

pantoniou · 2023-01-31T07:52:32Z

include/libfyaml.h

+		__res = NULL;					\
 								\
 		if (__str) {					\
-			if (__len == FY_NT) 			\


Lots of places with trailing blanks on a line.

pantoniou · 2023-01-31T07:52:35Z

include/libfyaml.h

 * If the _str pointer is NULL, then NULL will be returned
 */
 #ifndef FY_ALLOCA_COPY_FREE
-#define FY_ALLOCA_COPY_FREE(_str, _len)				\


I would like to keep the binary API and not break it.

Changing all alloca() using macros to take an extra argument breaks that.

The way this can be resolved is to define a macro with the same name and a leading underscore taking the extra argument and leaving the original macro conditional on GCC/CLang define block

for example for FY_ALLOCA_COPY_FREE

#define _FY_ALLOCA_COPY_FREE(_str, _len, _res) /* your definition */ #ifdef __FY_COMPILER_SUPPORTS_GCC_EXTENSIONS #define FY_ALLOCA_COPY_FREE(_str, _len) \ ({ \ char *_res; \ _FY_ALLOCA_COPY_FREE(_str, _len, _res); \ _res; \ })

pantoniou · 2023-01-31T07:53:02Z

include/libfyaml.h

 			   enum fy_emitter_cfg_flags flags)
 	FY_EXPORT;

-#define fy_emit_document_to_string_alloca(_fyd, _flags) \


No API changes please...

pantoniou · 2023-01-31T07:53:47Z

include/libfyaml.h

 fy_diagf(struct fy_diag *diag, const struct fy_diag_ctx *fydc,
 	 const char *fmt, ...)
 	FY_EXPORT
-	__attribute__((format(printf, 3, 4)));


This would be fine as a separate PR

pantoniou · 2023-01-31T07:55:23Z

src/internal/libfyaml-parser.c


 void print_escaped(FILE *fp, const char *str, int length)
 {
-	fprintf(fp, "%s", fy_utf8_format_text_a(str, length, fyue_doublequote));


This is an internal test tool, I don't think you care about much.
It is never installed in the system anyway.

It is fine to just disable building that in windows case.

pantoniou · 2023-01-31T07:56:34Z

src/lib/fy-diag.c

 	}

 	rc = fy_diag_printf(diag, "%s" "%*s" "%*s" "%*s" "%*s" "%s" "%s\n",
-			color_start ? : "",


GCC extension again.

pantoniou · 2023-01-31T07:57:06Z

src/lib/fy-diag.h

 	FYDB_TOKEN_DIAG(_fydb, _fyt, FYET_WARNING, _module, _fmt, ## __VA_ARGS__)

 /* alloca formatted print methods */
-#define alloca_vsprintf(_fmt, _ap) \


same method as described in libyaml.h

pantoniou · 2023-01-31T08:01:00Z

src/lib/fy-parse.c

 	size_t tlength;
 #endif

+    fy_utf8_format_a(c, fyue_singlequote, &str);


Yeah, that's not good.

You unconditionally format the error string even when no error happens.
This is on the hot path, and will slow down things.

We would probably have to do an open coded if () followed by a call to fyp_error()

pantoniou · 2023-01-31T08:03:17Z

src/lib/fy-parse.c


 #include <stdio.h>
 #include <string.h>
+#if defined (__unix__) || (defined (__APPLE__) && defined (__MACH__))


probably group all these together

pantoniou · 2023-01-31T08:05:44Z

src/lib/fy-token.c

 		outsz = *outszp;
 		o = out;
-		oe = out + outsz;
+		oe = (char *)out + outsz;


I get this why it needs to happen but I still dislike MSVC here.

pantoniou · 2023-01-31T08:09:36Z

src/lib/fy-utils.c

+#define INIT_SZ 128
+
+int
+vasprintf(char **str, const char *fmt, va_list ap)


Maybe you're over doing it here.

IIRC vsnprintf with a NULL string returns the exact size, so you can just call it twice, first call to find the size, second call after allocating it.

pantoniou

Separate PR with FY_ATTRIBUTE changes only

pantoniou

Maybe squash all patches and generate different PRs

Enjection · 2023-02-02T12:11:03Z

Wow, thanks for review, it was PoC to build under MSVC and I didn't think you would review it until I publish it (now it is a draft). I will spend some time to separating changes and fixing your comments on these weekend.

God-damnit-all · 2023-09-27T01:31:54Z

@Enjection If it's not too much trouble, could you give a status update?

Enjection added 7 commits January 29, 2023 00:06

replace unportable gcc extensions

7c01e0b

replace unportable gcc extensions

67a4075

fix unportable headers

55f7773

small fixes

16fcfcf

fix __attribute__ in diag

308d127

fix __attribute__

cc6fb5d

fix ptr arithmetics

bf81e2b

pantoniou requested changes Jan 31, 2023

View reviewed changes

ximion mentioned this pull request Oct 8, 2025

libappstream is not cross-platform anymore ximion/appstream#735

Open

Add basic win support #67

Are you sure you want to change the base?

Add basic win support #67

Uh oh!

Conversation

Enjection commented Jan 28, 2023

Uh oh!

pantoniou commented Jan 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pantoniou left a comment

Choose a reason for hiding this comment

Uh oh!

pantoniou left a comment

Choose a reason for hiding this comment

Uh oh!

Enjection commented Feb 2, 2023

Uh oh!

God-damnit-all commented Sep 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants