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.

Sign up for GitHub

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

Jump to bottom

Add basic win support #67

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Add basic win support #67

wants to merge 7 commits into from

Conversation

Enjection
Copy link

No description provided.

@pantoniou
Copy link
Owner

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
pantoniou requested changes Jan 31, 2023
View reviewed changes
include/libfyaml.h
@@ -41,6 +41,11 @@ extern "C" {

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

Choose a reason for hiding this comment

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

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

include/libfyaml.h
@@ -71,9 +76,11 @@ struct fy_document_iterator;
#define FY_NT ((size_t)-1)

#if defined(__GNUC__) && __GNUC__ >= 4
#define FY_ATTRIBUTE(attr) __attribute ((attr))
Copy link
Owner

Choose a reason for hiding this comment

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

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

include/libfyaml.h
\
if (__str) { \
if (__len == FY_NT) \
Copy link
Owner

Choose a reason for hiding this comment

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

Lots of places with trailing blanks on a line.

include/libfyaml.h
@@ -84,34 +91,32 @@ struct fy_document_iterator;
* If the _str pointer is NULL, then NULL will be returned
*/
#ifndef FY_ALLOCA_COPY_FREE
#define FY_ALLOCA_COPY_FREE(_str, _len) \
Copy link
Owner

Choose a reason for hiding this comment

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

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; \
  })

include/libfyaml.h
@@ -2022,8 +2027,8 @@ fy_emit_document_to_string(struct fy_document *fyd,
enum fy_emitter_cfg_flags flags)
FY_EXPORT;

#define fy_emit_document_to_string_alloca(_fyd, _flags) \
Copy link
Owner

Choose a reason for hiding this comment

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

No API changes please...

include/libfyaml.h
@@ -5237,22 +5242,9 @@ int
fy_diagf(struct fy_diag *diag, const struct fy_diag_ctx *fydc,
const char *fmt, ...)
FY_EXPORT
__attribute__((format(printf, 3, 4)));
Copy link
Owner

Choose a reason for hiding this comment

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

This would be fine as a separate PR

src/internal/libfyaml-parser.c
@@ -144,7 +144,9 @@ static void display_usage(FILE *fp, char *progname)

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

Choose a reason for hiding this comment

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

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.

src/lib/fy-diag.c
@@ -508,13 +531,13 @@ int fy_vdiag(struct fy_diag *diag, const struct fy_diag_ctx *fydc,
}

rc = fy_diag_printf(diag, "%s" "%*s" "%*s" "%*s" "%*s" "%s" "%s\n",
color_start ? : "",
Copy link
Owner

Choose a reason for hiding this comment

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

GCC extension again.

src/lib/fy-diag.h
@@ -686,8 +686,8 @@ void fy_document_builder_diag_report(struct fy_document_builder *fydb,
FYDB_TOKEN_DIAG(_fydb, _fyt, FYET_WARNING, _module, _fmt, ## __VA_ARGS__)

/* alloca formatted print methods */
#define alloca_vsprintf(_fmt, _ap) \
Copy link
Owner

Choose a reason for hiding this comment

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

same method as described in libyaml.h

src/lib/fy-parse.c
#ifdef ATOM_SIZE_CHECK
size_t tlength;
#endif

fy_utf8_format_a(c, fyue_singlequote, &str);
Copy link
Owner

Choose a reason for hiding this comment

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

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
pantoniou requested changes Jan 31, 2023
View reviewed changes
src/lib/fy-parse.c
@@ -11,12 +11,18 @@

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

Choose a reason for hiding this comment

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

probably group all these together

src/lib/fy-token.c
@@ -159,7 +159,7 @@ static int fy_tag_token_format_internal(const struct fy_token *fyt, void *out, s
if (out) {
outsz = *outszp;
o = out;
oe = out + outsz;
oe = (char *)out + outsz;
Copy link
Owner

Choose a reason for hiding this comment

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

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

src/lib/fy-utils.c
#define INIT_SZ 128

int
vasprintf(char **str, const char *fmt, va_list ap)
Copy link
Owner

Choose a reason for hiding this comment

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

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
pantoniou requested changes Jan 31, 2023
View reviewed changes
Copy link
Owner

@pantoniou pantoniou left a comment

Choose a reason for hiding this comment

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

Separate PR with FY_ATTRIBUTE changes only

pantoniou
pantoniou requested changes Jan 31, 2023
View reviewed changes
Copy link
Owner

@pantoniou pantoniou left a comment

Choose a reason for hiding this comment

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

Maybe squash all patches and generate different PRs

@Enjection
Copy link
Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pantoniou pantoniou pantoniou requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Enjection @pantoniou @God-damnit-all