search

valum-framework / valum

Web micro-framework written in Vala
https://valum-framework.readthedocs.io/en/latest/
GNU Lesser General Public License v3.0
225 stars 23 forks source link

Enable experimental non-null #181

Open Bob131 opened 8 years ago

Bob131 commented 8 years ago

Fixes #37

This patch set just goes through the compiler errors and fixes each of them. I'm yet to add a final commit adding the flag to the meson config file since there's two files that don't seem trivial enough for me to handle on my own: src/valum/valum-router.vala and src/valum/valum-static.vala. An advisory on how to proceed with those would be appreciated.

All the commits are pretty trivial in nature, at worst there's minor changes to control flow. I've tried to document my changes in the commit message bodies as best I can, feedback welcome

arteymix commented 8 years ago

Nice work! I'll review the changes when you feel it's ready.

In the meantime, you can add --experimental-non-null to test it on the CI. Hope you're not too confused with the new build system ;)

codecov-io commented 8 years ago

Current coverage is 68.25%

Merging #181 into master will decrease coverage by 6.00%

@@             master       #181   diff @@
==========================================
  Files            24         24          
  Lines           765        778    +13   
  Methods           0          0          
  Messages          0          0          
  Branches         89         91     +2   
==========================================
- Hits            568        531    -37   
- Misses          197        221    +24   
- Partials          0         26    +26

Powered by Codecov. Last updated by 9c68c05...2d55149

Bob131 commented 8 years ago

Er, okay. So it appears as though the ancient vapis used in the Travis builder bind libsoup with some incorrect function sigs. I'm not sure what to do about that. For my own projects I'd normally just ship a patched vapi; is that appropriate here?

Bob131 commented 8 years ago

Oh, nvm. Sorry about that, I hadn't actually thought of using type coercion to fix that. I'll get back to you soon re extra changes

Bob131 commented 8 years ago

Alright, there we go. So that build fails (starting at https://travis-ci.org/valum-framework/valum/builds/134095170#L748) because I haven't touched valum-router.vala and valum-static.vala. I've never looked at (or even knew about) runtime flag introspection and boolean operators are still kinda new to me, so that's why I've ignored the former. As for the latter, I'm not familiar enough with Valum to know what's really going on, eg I don't know whether certain cxt members are guaranteed not to be null or whether it's a failure mode, or how failures should be handled in that closure.

Besides those two files remaining problematic, the rest of the patch set is good for review (or at least I think so, hopefully I didn't forget anything ;)

arteymix commented 8 years ago

Is it necessary to cast expression like nullable ?? default? If so, Vala need some type inference work.

Bob131 commented 8 years ago

Unfortunately it is necessary to cast such expressions. It's a bit of an eyesore, but I find syntactical inconveniences like that to be well worth the added safety. Not that your code seems to have suffered too much!

It may well be a bug in valac which I might investigate at some point, but my experience of the Vala project on GNOME's Bugzilla has been pretty lacklustre.

arteymix commented 8 years ago

Casting to non-null is not a safer approach and may actually hide bugs. Maybe you should look at the possibility of introducing explicitly typed variables? Something like..

string? nullable_a = null;
string a = nullable_a ?? "defaut";

In the past, I attempted to work on this, but I resigned because it introduced bugs. It seems like the (!) operator is not idempotent.

Bob131 commented 8 years ago

What I was trying to say was that the second example doesn't work, you must cast an expression with the ?? operator to non-null (as little sense as that makes). Of course the (!) cast can be dangerous and discretion must be exercised with its use; if you notice a patch where I haven't taken due care, please let me know. Otherwise, I'm not sure what you're talking about.

arteymix commented 8 years ago

It was more a general idea to avoid the (!) operator by introducing an explicitly typed variable. Likewise I think that it would be important to use it only of necessary because it hides potential bug.

arteymix commented 8 years ago

I really think we should postpone that until we get better support for null-checking in the compiler. It wouldn't hurt to apply the --enable-experimental-non-null flag from a specific version of valac.

Bob131 commented 8 years ago

Sounds good to me.

On Fri, Jun 3, 2016 at 2:44 PM, Guillaume Poirier-Morency notifications@github.com wrote:

I really think we should postpone that until we get better support for null-checking in the compiler. It wouldn't hurt to apply the --enable-experimental-non-null flag from a specific version of valac.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.