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.

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

Compilation under Visual Studio 2010 #63

Open
end2endzone opened this issue Sep 7, 2020 · 6 comments
Open

Compilation under Visual Studio 2010 #63

end2endzone opened this issue Sep 7, 2020 · 6 comments

Comments

@end2endzone
Copy link

Hi.

I apologize if my English is not always clear. English is not my native language and I sometime lack using the proper word.

I plan to integrate an expression parser in one of my projects. I have taken a look at tinyexpr and think that it is a really nice library. I found multiple compilation errors when compiling with Visual Studio 2010. I do realize vs2010 is old but this is my development environment for my project. I would be really happy if you would consider supporting vs2010.

When compiling revision ffb0d41, the following errors were identified by Visual Studio:

1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(124): error C2124: divide or mod by zero
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(127): error C2143: syntax error : missing ';' before 'type'
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(129): error C2065: 'i' : undeclared identifier
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(256): error C2223: left of '->type' must point to struct/union
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(403): error C2275: 'te_expr' : illegal use of this type as an expression
1>          f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.h(39) : see declaration of 'te_expr'
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(408): error C2059: syntax error : '{'
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(487): error C2371: 'expr' : redefinition; different basic types
1>          f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(297) : see declaration of 'expr'
1>f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.c(596): error C2275: 'te_expr' : illegal use of this type as an expression
1>          f:\projets\programmation\cpp\tinyexpr\master\tinyexpr.h(39) : see declaration of 'te_expr'

(I only added one example of each error)

The full error log can be found here: Compilation of ffb0d41b13e5f8d318db95feb071c220c134fe70 under VS2010.txt

Most of these errors are not critical and could be easily resolved by adding the declarations of variables at the beginning of the functions instead of after if statements. I hope you are open to suggestion and that you would like to support this compiler. I also wish that you would not count this change "as a new feature" and that it does not break your contributing rules and keep tinyexpr as simple as possible.

I have already take a look at most of the compilation issues. Here are the corrections to solve all issues:

  1. The macros NAN and INFINITE are undeclared identifier. On VS2010, these macros are not defined. The solution would be something like specified at https://gitlab.com/embeddable-common-lisp/ecl/-/issues/282:
#include <stdint.h>

#ifndef INFINITY
#if _MSC_VER == 1600
union {
	uint8_t bytes [ sizeof ( float ) ];
	float inf;
} __ecl_inf = {
	{ 0, 0, 0xf0, 0x7f }
};
#define INFINITY (__ecl_inf.inf)
#else
# define INFINITY (1.0/0.0)
#endif
#endif

#ifndef NAN
#if _MSC_VER == 1600
union {
	uint8_t bytes [ sizeof ( float ) ];
	float nan;
} __ecl_nan = {
	{ 0, 0, 0xc0, 0x7f }
};
#define NAN (__ecl_nan.nan)
#else
# define NAN (0.0/0.0)
#endif
#endif
  1. The malloc() call at tinyexpr.c, line 88 should be prefixed with (te_expr *) to cast void* to te_expr *.

  2. The function fac() needs the declarations of the following at the beginning of the function:

    unsigned int ua;
    unsigned long int result, i;
  1. The function ncr() needs the declarations of the following at the beginning of the function:
    unsigned long int un, ur, i;
    unsigned long int result;
  1. The function next_token() needs the declarations of the following at the beginning of the function:
    const te_variable *var = NULL;
  1. The declaration of the ret variable in function in function power() at tinyexpr.c, line 403 needs to be moved at the beginning of the function.

  2. The declaration of the root variable in function in function te_compile() at tinyexpr.c, line 596 needs to be moved at the beginning of the function.

  3. Expression such as te_fun2 t = s->function; need to be type casted to compile properly. This change affects lines 431, 460, 477 and 492. The result would be the following:

        te_fun2 t = (te_fun2)s->function;
  1. Visual Studio 2010 do not support ... syntax in a define statement like the one at tinyexpr.c, line 82.
    This one is more complicated to fix and the fix is less elegant than actual code but please consider to use these corrections. I managed to fix these errors by declaring a macro for each variant of the number of argument:
#define NEW_EXPR1(type, expr1) new_expr1((type), (expr1))
#define NEW_EXPR2(type, expr1, expr2) new_expr2((type), (expr1), (expr2))

static te_expr *new_expr1(const int type, te_expr *param1) {
  const te_expr *parameters[] = {param1};
  return new_expr(type, parameters);
}

static te_expr *new_expr2(const int type, te_expr *param1, te_expr *param2) {
  const te_expr *parameters[] = {param1, param2};
  return new_expr(type, parameters);
}

and replacing usage of the NEW_EXPR() macro by NEW_EXPR1() or NEW_EXPR2(). For example, replace line 494 by the following:

        ret = NEW_EXPR2(TE_FUNCTION2 | TE_FLAG_PURE, ret, power(s));

Since the number of calls to the NEW_EXPR macro is limited to 5 lines of code with 2 or 3 argument variants, I think this does not hurt the code too much.

I have never contributed to a github project but if you are open to these corrections, I can modify the code and create a pull request if you wish.

Looking forward for your feedback.

@codeplea
Copy link
Owner

codeplea commented Sep 8, 2020

Thanks for sharing your fixes. If any others are in your situation, I'm sure they will find it very useful. You did a great job of documenting exactly where the changes are needed.

That said, I'm not sure it makes sense to support VS 2010. VS had/has a lot of idiosyncrasies back then, and I don't think it's unreasonable to insist that a user of tinyexpr should have at least a C99 compliant compiler.

I guess my question to you would be, why are you using a 10 year old compiler? C99 was standardized 20 years ago. Don't you miss the features it has available?

I'll leave this issue open for a while in case anyone else wants to comment on it.

@end2endzone
Copy link
Author

Hi. Thank you for your fast response. It is much appreciated.

I hope the proposed changes will be useful to someone else. If not, that will be because we are very very few to still work on VS2010.

Even if I disagree about support for VS2010, I do understand your considerations. You must also trace a line and that line is to have at least a C99 compliant compiler.

As to why I still use VS2010, it is because I created my setup 10+ years ago. It was the setup that I used for work too. I mostly develop on Windows for Windows. I don't miss the new c++ standards too much. Some c++11 features would be great though. I never took time to upgrade my system to a newer version because my ssd is almost full and licensing fees. I guess it is time to update to a new system now...

Since your decision has been made and is unlikely to change, feel free to close the issue when you feel like it.

Regards.

@EvilPudding
Copy link
Contributor

I think supporting C89 would be pretty cool. If the differences are mostly aesthetic like variable declarations on the top of the block, being more compatible would come at no cost. I don't like the macros checking the msc version specifically as it wouldn't help if you're using any other compiler.

@end2endzone
Copy link
Author

Regarding the macros, I agree that multiple #ifdef is ugly. What I can do is to define NAN and INFINITE on my side before I include tinyexpr.h and since your code is already using #ifndef NAN, it will be transparent from your side. I actually only need the changes regarding NEW_EXPR, having the variable declarations at the beginning of the functions and the c-style casting for s->function and malloc() calls.

As far as I can remember, when I used to program in c, I had to have all variable declarations at the beginning. This might be a Visual Studio only thing or it might have changed in a "not so recent" standard.

@codeplea
Copy link
Owner

codeplea commented Sep 8, 2020

I suppose it wouldn't be too hard to support it. You did all the work already anyway.

There may be others with embedded compilers still on C89 as well.

I'm going to think about it.

@bavay
Copy link
Contributor

bavay commented May 25, 2021

Although I fully agree with the reluctance to support MS' idioms (as found in VC++10) when there are clearly defined standards, I have actually encountered a case where C89 support would have been great. No big surprise, it is still coming from Redmont: people would download Visual Studio Express don't have c99 support and only get a screen full of error messages when trying to compile tinyexpr. This makes no sense, I really don't understand MS' strategy and it is very confusing for the users who want to recompile software themselves because the not "Express" versions properly support c99... So because of this illogical choice of Microsoft, supporting compiling with c89 only would still be useful even in 2021.

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

No branches or pull requests

4 participants