Time Nick Message 01:26 est31 hmmmm, time for the review? 01:27 hmmmm yeah sure 01:30 hmmmm anybody else want to join in? 01:39 hmmmm est31: you did test this on android right? 01:39 est31 should I? 01:40 hmmmm there is a lot of android build code 01:40 hmmmm i have no idea if any of it is correct or not 01:40 est31 ah yes I tested the building part 01:40 hmmmm ok 01:40 est31 (one of nrzkt's bots also tests it) 01:57 hmmmm hrmm 01:57 hmmmm did you ever consider changing AUTH_MECHANISM_FIRST_SRP to, say, AUTH_MECHANISM_CREATE_ACCOUNT or something clearer? 01:57 hmmmm AUTH_SRP_CREATE_ACCOUNT 01:58 hmmmm AUTH_SRP_LOGIN 01:58 hmmmm AUTH_UPGRADE_LEGACY_TO_SRP 01:58 hmmmm idk 01:58 hmmmm i think that adding the word MECHANISM to those enum values might be extra-verbose 01:58 hmmmm and the space could be used to provide more/better description 01:59 est31 ok 01:59 hmmmm another thing, pointer values = NULL 01:59 hmmmm not 0! 01:59 est31 can you comment that in gituhub? 02:00 est31 about FIRST_SRP vs CREATE_ACCOUNT: its also used for changing the password 02:00 hmmmm sure 02:00 hmmmm ahhh okay 02:00 hmmmm makes more sense then 02:00 hmmmm it'd probably be a good idea to add a comment explaining exactly which each of these mechanisms is then 02:00 hmmmm s/is/are/ 02:00 hmmmm also you can have multiple mechanisms per request??? 02:01 est31 no 02:01 hmmmm i'm confused about get_auth_mech 02:01 est31 changing passwords is done otherwise 02:01 est31 ah 02:01 hmmmm I need to actually check out your branch hold on 02:01 est31 first about the coments: there are comments 02:01 hmmmm so then i can goto definitions of things 02:01 est31 https://github.com/minetest/minetest/pull/2620/files#diff-d6c71fb6c2c5e2ad979f39a0422f7786R909 02:08 hmmmm est, about AuthMechanism 02:08 hmmmm I see you're trying to use it like a set of flags 02:09 est31 yes 02:09 hmmmm but the second you combine at least two enum values it becomes an invalid value 02:09 hmmmm AUTH_MECHANISM_SRP | AUTH_MECHANISM_LEGACY_PASSWORD is not in the enum 02:10 est31 perhaps I should store the result in a u32 where I do that 02:11 hmmmm why the free((char *) salt)? 02:12 hmmmm first the style is to not have a space between unary operators and their operands 02:12 est31 because itget_auth_mech 02:12 hmmmm second, you don't need to cast it at all 02:12 est31 oh sorry 02:12 hmmmm then m_authstate = 1; 02:12 est31 really? 02:12 hmmmm yeah 02:13 hmmmm in any case all these are minorish problems that can be addressed as we go along 02:14 est31 casts are really without space? 02:14 hmmmm yeah 02:14 est31 I like them more with space 02:14 hmmmm well 02:14 hmmmm it's not a dealbreaker 02:14 est31 yes 02:14 hmmmm i'm just saying what the rest of the code uses 02:14 est31 ok 02:15 hmmmm (char *)foo is an expression consisting of the identifier 'foo' as the operand of the unary operator (char *) 02:15 hmmmm much like ! or ~ 02:15 hmmmm ~foo 02:15 hmmmm you don't write ~ foo, now do you :)? 02:15 est31 yes 02:15 Zeno` haha 02:15 est31 nor do I write ++ i 02:15 hmmmm I know some people who do 02:16 hmmmm :( 02:16 hmmmm at work we have code reviews and I have a particularily large lump of code that happened because our buildbot was broken for the longest period of time on the repo i needed to commit to 02:16 hmmmm and now it's being reviewed too and there are like 100+ comments 02:16 hmmmm the more you look at it, the more delay 02:17 hmmmm because there's always SOMETHING 02:17 hmmmm i think i would advocate pushing it as-is, as long as we don't find like horrible deadly bugs that cause bluescreens or likewise 02:18 hmmmm and then things get fixed as we go along 02:18 hmmmm what do the rest of you guys think 02:19 hmmmm right now I'm just trying to understand all the paths of this code and the overall structure 02:21 Zeno` I think the basic style issues should be fixed before merging, though 02:21 Zeno` e.g. https://github.com/minetest/minetest/pull/2620/files#diff-34f48ad91ac6c202ac60b0348ae90e30R1019 02:21 hmmmm if I sat here and enumerated the style issues there would be like 50000 02:21 hmmmm eww yea that's gross 02:21 Zeno` I don't know about enclosing case's within {} (a compound statement) 02:22 est31 yes I did it because I declared variables inside the case 02:22 hmmmm doing /that/ is fine, but putting the next case on the same line as the previous closing bracket is not 02:22 hmmmm there are a lot of levels of review 02:22 Zeno` I'm just unsure about the break being inside said compound statement 02:23 hmmmm review for the code style, review of language idioms used, review of the code structure and approach, review for potential bugs and exploits, etc. 02:23 Zeno` to me something subtle could go awry that would be difficult to find 02:23 hmmmm sure 02:23 hmmmm as you encounter things that are weird let's at least mark them down in the pull request 02:23 Zeno` e.g. see Duff's Device on the intricacies of C's switch 02:24 hmmmm so it's not forgotten about 02:24 hmmmm duff's device? 02:24 Zeno` http://en.wikipedia.org/wiki/Duff%27s_device 02:24 hmmmm is that the one where it effectively pulls off a loop inside ofa switch statement 02:24 Zeno` yeah 02:24 hmmmm yeah hahaha 02:24 hmmmm oh man that's crazy 02:24 Zeno` but it's not *obvious* how the switch works 02:24 hmmmm as long as it's well commented I'd accept that, considering the performance benefit :-) 02:25 Zeno` well, yeah :) 02:25 Zeno` I'm using it as an example of how switch/case is not always obvious hehe 02:25 hmmmm that guy shouldve s/% 8/& 7/ 02:25 hmmmm s/ \/ 8/ >> 8/ 02:25 hmmmm erm, >> 3 02:26 est31 yes 02:26 Zeno` haha 02:26 sofar *** Error in `bin/minetest': double free or corruption (!prev): 0x0882b8c8 *** 02:26 sofar just... starting it up 02:26 sofar crashed immediately 02:26 hmmmm sofar, can you reliably reproduce that? 02:26 est31 thats natural for minetest 02:26 sofar $ git describe --tags 02:26 sofar 0.4.11-489-g6ec3163 02:26 Zeno` sofar, you're not using valgrind or something are you? 02:26 est31 the problem starts when its reproducable 02:26 sofar Zeno`: not right now :) 02:26 Zeno` ok 02:27 sofar I'll do some startup cycling and see if it comes again 02:27 hmmmm sofar, what platform is that? 02:28 sofar linux 32bit 02:28 sofar self-compiled, like most of my software stack :) 02:28 hmmmm hmm 02:29 sofar I've done at least 50+ starts with this build already since I use that to debug / work on my mods 02:29 sofar so it's a rare thing 02:29 est31 I'm having it too, every now and than 02:29 est31 then* 02:29 est31 issue is, how do you debug when you can't reproduce? 02:31 _X_C_V_B_ is minetest web scale 02:33 Zeno` est31, tbh I'd make those two switch cases a function each :) 02:33 Zeno` thanks Mr ShadowBot 02:35 est31 Zeno`, the problem with functions would be that you have to add them in the header too 02:35 est31 and thats bad:( 02:35 Zeno` why? 02:35 est31 because they are nonstatic 02:35 est31 they use the client's instance 02:35 est31 ofc I could give the client as pointer 02:36 est31 but dunno of that would still be cool 02:36 Zeno` having them private or protected is bad? 02:36 est31 if* 02:36 est31 no 02:37 Zeno` or yeah you could have them as a static function (not member function) in the C++ file an pass 'this' 02:37 Zeno` I don't see much that's awful about that 02:37 _X_C_V_B_ mongodb is webscale 02:37 _X_C_V_B_ so, is minetest web scale???? 02:37 sofar it's not 02:38 est31 Zeno`, yea perhaps 02:38 est31 can you add a github line note? 02:38 est31 (I won't fix this right now too tired) 02:38 Zeno` ok 02:39 _X_C_V_B_ I'm going to make my own open source version of minecraft 02:39 sofar cheers, please talk somewhere else 02:40 _X_C_V_B_ o 02:40 _X_C_V_B_ ok 02:42 sofar o_O that... worked 02:42 Zeno` est31, m_password == "" 02:43 hmmmm hrmm 02:43 Zeno` I know it *might* seem hacky but what would be wrong with simply (u8)(m_password == "") ? 02:43 Zeno` i.e. true *is* 1 already 02:44 hmmmm okay 02:44 est31 yea 02:44 est31 is false automatically 0? 02:44 Zeno` yes, but see my line comment as well 02:44 hmmmm so m_auth_data is an SRPUser * in the case of auth mechanism == *_SRP 02:45 hmmmm and then it *can* be something different? 02:46 est31 yes 02:46 hmmmm if I were coding this I would probably use some C++ features and make AuthMethod or something into a base class 02:46 hmmmm and then have AuthMethodSRP, AuthMethodLegacy, etc. 02:46 hmmmm so a fundamentally different approach 02:46 hmmmm you have things more flattened out rather than structured by class 02:46 est31 yea I've thought of that but then I remembered of how you liked c++ 02:46 hmmmm well 02:46 hmmmm I like things that aren't overly complicated unless they need to be 02:47 hmmmm you should do it however you want to have it and then unless it's absolutely horrible, we change it 02:47 hmmmm errm, we keep it * 02:47 hmmmm but why isn't m_chosen_auth_mech an AuthMechanism? 02:49 est31 because I didn't want to include the header just for that 02:49 est31 perhaps its included anyways 02:49 est31 add a line note, I'll change it if it still compiles 02:49 hmmmm well I'm adding line notes for everything 02:49 hmmmm just so i don't forget it later 02:50 hmmmm but i'm most concerned about 1). no bugs, and 2). good structure 02:50 Zeno` oh man 02:50 Zeno` here I am adding pedantic comments and NOW you say that 02:50 hmmmm no 02:50 hmmmm you can add pedantic comments too 02:51 hmmmm it's just that they're not as high in terms of priority 02:51 Zeno` of course not :) 02:51 Zeno` heh 02:51 hmmmm I have to deal with this one guy at work who is the most stubborn person in the universe 02:51 est31 about the separate classes: currently the handlers are at one single place 02:51 hmmmm and one missing space is like the end of the world 02:52 hmmmm and refuses to review anything else unless all trivial style "issues" are done 02:52 est31 so it would perhaps be confusing to separate it into other files/classes 02:52 hmmmm it's so annoying so I try to not replicate that 02:52 Zeno` Finally somebody uses 1 << 0, 1 << 1 etc instead of 0x1, 0x2, 0x4, 0x8, 0x10, 0x20 02:52 hmmmm at the end of the day, i have literally never seen him review code 02:53 hmmmm yeah but having flags in an enum defeats the purpose 02:53 est31 SN's idea. 02:53 Zeno` I'm not allowed to add positive comments though 02:53 est31 lol 02:53 Zeno` ;D 02:53 est31 you can add them here :p 02:53 Zeno` already have 02:53 hmmmm there should be a Flags data type in some language 02:54 est31 the first implementation of this used strings with every auth mechanism being a byte 02:54 hmmmm what why :/ 02:54 est31 when I got into issues to get it actually working I dumped it 02:55 est31 strings might have unlimited length (so unlimited number of auth mechs) 02:55 est31 but they are clunky 02:55 hmmmm rolls eyes 02:55 hmmmm yeah like we're going to have more than 42795494396 auth mechanisms 02:55 est31 and I had those issues, so I used the c way 02:56 est31 right now 32 are possible 02:56 est31 thats enough 02:59 Zeno` https://github.com/est31/minetest/blob/srplogin/src/network/serverpackethandler.cpp#L184 02:59 Zeno` what's that for? 02:59 Zeno` A synonym for playerName ? 02:59 est31 SN's casing changes 03:00 est31 makes the protocol work for userHello, when they first registered as UsErHellO 03:00 est31 so right now both client and server are ignoring it. 03:01 Zeno` I don't like that... 03:01 est31 but when we want to add it later, its there 03:01 hmmmm lots of game networks have that case insensitivity 03:01 Zeno` it looks too much like implementation defined behavior 03:01 hmmmm are minetest player names currently case insensitive?? 03:01 est31 no 03:01 est31 there is a PR for that somewhere 03:01 Zeno` (storing the pointer to the c_str() and then changing playerName I mean) 03:02 hmmmm hrmm 03:02 hmmmm yeah I think that may be undefined behavior 03:02 hmmmm i wouldn't know without consulting the standard 03:02 est31 where do I do that? 03:03 Zeno` hmm 03:03 Zeno` line 165 and line 184 are what I'm wondering about 03:04 Zeno` it just seems odd 03:04 est31 isn't the assignment the other way round? 03:06 Zeno` Maybe. I'm only glancing, but if the meaning isn't clear at a glance I don't like it :) i.e. if something needs to be copied to be changed then it should be copied and changed in the lines immediately following 03:07 Zeno` and on line 165 I don't know why you're storing the pointer returned by .c_str() 03:07 Zeno` maybe it's for convenience? 03:07 est31 yes 03:07 Zeno` (I could look through of course, but I'm lazy) 03:07 est31 nono only convenience 03:08 est31 also to keep diff small 03:08 Zeno` I guess maybe the comment immediately above it is misleading in that case 03:09 Zeno` Ok, what if somebody *does* change playerName after you store that pointer? 03:09 est31 perhaps I should swap lines 182 and 183? 03:10 Zeno` Not sure what the best way to approach it is. What I'm saying is that it could lead to bugs... 03:10 Zeno` even if there are not currently any 03:10 est31 yea perhaps I should add //for convenience to 164 03:10 est31 yes bug prevention is one of the things style tries to achieve 03:11 Zeno` perhaps... or that *and* move it closer to where it's being used for convenience 03:13 est31 add it as nore 03:13 est31 note* 03:13 est31 sorry nore :D 03:15 Zeno` that whole function probably needs tidying up (and you probably added it in haste because it looks different to the rest of your changes heh) 03:16 hmmmm gosh now more than ever I realize how much I hate networkpacket 03:17 est31 ? 03:17 hmmmm is it actually necessary to overload <> blah >> foobar >> asdf; 03:36 est31 thats what c++ has static typing for 03:37 hmmmm this is painful to look at 03:37 est31 its much shorter 03:37 hmmmm shorter IS NOT NECESSARILY BETTER 03:38 hmmmm we could also write our code like 03:38 hmmmm if(a+b>34){ 03:38 hmmmm d.crnlz(); 03:38 hmmmm but i'm pretty sure that would make people want to stab things 03:38 Zeno` est31, you run this on your server? 03:39 est31 no Zeno` that server runs stable 0.4.12 :D 03:39 Zeno` ok, just thought you did 03:39 hmmmm what is a C String used for in the protocol again?? 03:40 est31 ? 03:40 hmmmm putCString() 03:40 est31 ah 03:40 est31 its the same as << std::string(ptr, len) 03:40 est31 spares a copy 03:40 hmmmm yeah but why 03:40 hmmmm what's wrong with the pascal-style strings 03:41 est31 ? 03:41 est31 I've learned pascal once for uni, then forgot it again 03:42 hmmmm pascal style strings are how variable length strings in packets are currently serialize 03:42 hmmmm d 03:42 est31 yes the fun part is putCString() does really exactly the same 03:42 hmmmm (u16 string length) (u8 characters[]) 03:42 est31 so it also adds the length 03:42 est31 thats why I said its the same as 03:42 hmmmm oh 03:43 est31 https://github.com/est31/minetest/blob/srplogin/src/network/networkpacket.h#L49 03:43 hmmmm then that's not a C string.. 03:43 Zeno` pascal strings are not nul terminated though 03:43 hmmmm in fact you specify src and len 03:44 hmmmm you just want to do a zero-copy string insertion 03:44 Zeno` (well, they might be but the standard wouldn't say they have to be) 03:44 Zeno` that is if there is a pascal standard lol 03:44 hmmmm do you really have to define a new function for this? 03:44 hmmmm i mean it's such a microoptimization.. 03:44 est31 I've thought its useful at other points too 03:45 est31 but yea that code isn't really performance relevant 03:45 hmmmm i don't know this is so fubar 03:45 hmmmm I wouldn't be using this operator overloaded interface to begin with 03:45 hmmmm does anybody aside from you and nerzhul like it? 03:46 est31 don't know 03:46 est31 Zeno`, do you like it? 03:46 hmmmm NetworkPacket::insertString(const std::string &); could be overloaded by NetworkPacket::insertString(void *, size_t) 03:47 hmmmm but moreover, we need to ask the more fundamental question what makes network packets so different from other types of serialization to justify having NetworkPacket to begin with 03:48 hmmmm how is any of this better than the istringstream-based interface we had before... :( 03:48 Zeno` like what? 03:48 Zeno` operator overloading used like that? 03:48 Zeno` nah, I don't 03:48 hmmmm you're lagging 03:49 Zeno` yeah, because I'm not watching hehe 03:49 est31 its easier this way than touching the raw bits and bytes 03:49 Zeno` have to go to Dr shortly and I'm doing about 8 things at once 03:49 est31 and less error probe 03:49 est31 prone* 03:49 hmmmm how is 03:49 est31 protobuf for the poor 03:49 hmmmm packet.data.insertU32(some_value); more error prone than pkt << some_value; 03:49 hmmmm what happens if we have 03:49 hmmmm int some_value;? 03:50 est31 istringstream has insertu32? 03:50 Zeno` the thing I don't like about pkt << some_value is what happens if some_value is not u32 and I forget to cast it? 03:50 hmmmm now you don't know which variation of NetworkPacket::operator << gets called unless you know what the size of int is on that architecture 03:50 hmmmm est31: no it doesn't, but that's how it was used before 03:50 hmmmm write32(is, foobar); 03:51 Zeno` if it's packet.data.insertU32() then it's always cast (implicitly) 03:51 hmmmm well this could just be modified to have a more OO-style interface and bam 03:51 est31 yes but you will have to make every single of those operations its own line 03:51 Zeno` so I don't have to worry about data types (well sizes really) at all 03:51 hmmmm so? 03:51 hmmmm that's a good thing 03:51 est31 which needlessly bloats the code 03:51 hmmmm no way 03:51 est31 I mean you already have all the type information you need 03:52 hmmmm I want things being sent on the wire to be very explicit 03:52 hmmmm I don't want it to be like 03:52 Zeno` you mean the compiler has it :) 03:52 hmmmm pkt<put$TYPE(a); is easier or $TYPE a; pkt << a; 04:09 Zeno` I'm doing too many things at once... need to go in 5 minutes. I think maybe I've been at "the wrong level of abstraction" if such a phrase exists 04:09 est31 thanks Zeno` and hmmmm for the review :) 04:09 Zeno` I was looking at things from the "inside" point of view where bytes count 04:09 hmmmm wait it's not done yet 04:09 Zeno` if we're outside the abstraction then everything I said above is incorrect 04:10 hmmmm [12:04 AM] why should networkpackets have to have a certain size? 04:10 hmmmm because reverse compatibility 04:10 hmmmm we have a very specific format that is to be followed, and if you do, we promise you that you'll be able to talk with minetest 04:11 est31 yes 04:11 est31 it has to remain stable I agree 04:12 est31 but whats better pkt >> a >> b >> c or a = pkt[1]; b = pkt[2]; c = pkt[3];? 04:12 est31 as long as we are doing the same as that code everything is well 04:12 est31 people should only then change things when they know whats happening 04:24 Zeno` anyway, back to the PR before I leave 04:25 Zeno` The only thing I strongly disagree with is the casts where casts are not needed 04:26 Zeno` e.g. https://github.com/minetest/minetest/pull/2620/files#diff-2d67c2347848dbd1d1d74e4b8d608694R93 04:26 Zeno` bbl 04:27 est31 thats nothing to argue about, its only me not knowing c good enough :) 04:58 hmmmm so on a scale from 1 to 10, what would you rate your C? 05:00 est31 tough question 05:00 hmmmm and how many other languages do you know? would you call yourself experienced? 05:01 hmmmm like what's your background 05:01 hmmmm it's difficult to judge you. on one hand it seems like you're very knowledgable, you know a lot about cryptography and such, but your C sucks :p 05:01 hmmmm so I don't consider you a novice 05:01 est31 I'm studying math thats perhaps why I know cryptography 05:02 est31 but I dont have that coding backgroung 05:02 est31 d* 05:02 hmmmm really? hm 05:02 hmmmm for no coding background you're pretty good 05:02 hmmmm keep up with it 05:02 hmmmm I was a math person but the career prospects for pure math frankly sucks, and applied math is pretty boring 05:02 hmmmm i wouldn't want to be an analyst 05:02 hmmmm or actuary 05:03 est31 yeah banking and insurance or so sucks 05:03 hmmmm that's the only place where jobs are 05:03 hmmmm I am so happy I ended up getting a software job in the end 05:04 est31 I'm aiming for software too 05:05 hmmmm i forgot a lot of math and i wish i didn't 05:06 hmmmm trying to write a unit test for PcgRandom::randNormalDist(), so obviously I'd need to do some kind of chisq goodness of fit, but to do that i need to know the variance, but i forgot how to figure out the variance of multiple independent trials :/ 05:07 est31 I've yet to hear this \sigma X = 1 stuff 06:08 est31 many things you point out hmmmm I know, I just missed them 06:08 est31 like identation 06:09 est31 or that pointers should be NULL 06:09 est31 that one I didn't deem too important 12:01 sfan5 https://github.com/minetest/minetest/issues/2656#issuecomment-96998742 12:01 sfan5 how relevant 12:03 deezl lol 12:04 deezl it does seem to defy logic, but there are many little things that stray a bit from reality...people seem to forget it is a game 17:02 Krock hmmmm, https://github.com/minetest/minetest/blob/master/src/unittest/test_random.cpp#L182 do you seriously mean 'i' or shouldn't there be an other variable? 17:02 Krock It is two times initialized 17:02 hmmmm shoot 17:03 hmmmm that is not good 17:03 Krock Hold on, I'm doing a pull to fix all mistakes 17:03 hmmmm but i'm surprised clang did not give a warning for shadowed variables 17:03 hmmmm Krock, don't bother....... 17:03 Krock well I do because there are round() issues 17:03 Krock in the same file 17:03 hmmmm what do you mean 17:04 Krock MSVC doesn't have round() use myround() instead 17:04 hmmmm nice 17:04 hmmmm I should fix all those at the same time 17:05 hmmmm i don't know if we should use myround() - does this work correctly for negative numbers? 17:07 Krock floor works with negaive, so yes. 17:08 Krock here some other tiny changes.. not worthy to create a commit for each one #2657 17:08 ShadowBot https://github.com/minetest/minetest/issues/2657 -- Fix several MSVC issues by SmallJoker 17:10 hmmmm floor doesn't work for -.5 17:10 hmmmm it rounds up 17:11 Krock I see 17:11 hmmmm oh didn't I remove that constructor? 17:11 Krock apparently you didn't 17:12 hmmmm by the way, what version of msvc are you using 17:14 Krock The CXX compiler identification is MSVC 16.0.30319.1 17:20 Krock so, the round problem is solved now 17:22 hmmmm it would be nice to have a small script that checks modifications before committing 17:23 hmmmm looks for accidental usages of uint32_t instead of u32, or round instead of myround, for example 17:23 est31 ? 17:23 Krock plsno 17:23 hmmmm things that could easily break msvc builds that people don't notice 17:23 Krock I wasn't ready with the CmakeLists.txt change 17:23 hmmmm huh?? 17:23 Krock see comments 17:24 * Krock headdesks 17:24 Krock nvm. 17:25 hmmmm was that causing a problem?? 17:25 est31 https://github.com/SmallJoker/minetest/commit/417a38c61e04d9f6e32defdb568892c7d8289c16 17:25 est31 look at cmakelists.txt 17:25 hmmmm can see that 17:26 Krock est31, he pushed an other commit 17:26 est31 yes 17:26 est31 but without cmakelists, no? 17:26 hmmmm erm 17:27 hmmmm doesn't that strip out all optimizations for jsoncpp 17:27 Krock dunno but without that line I can't compile minetest 17:27 hmmmm it should be 17:29 hmmmm wtf did somebody remove the flags for msvc in src/CMakeLists.txt?? 17:30 hmmmm oh guess not 17:30 Krock No, it's still there at line 564 17:30 Krock *562 17:31 hmmmm set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /MT" CACHE STRING "" FORCE) 17:31 hmmmm are you sure that it works for you without the CACHE STRING "" FORCE) ? 17:32 hmmmm for some reason CMake ignores my custom set CMAKE_CXX_FLAGS_RELEASE otherwise 17:32 Krock returns in " /MD /O2 /Ob2 /D NDEBUG /MT" 17:32 hmmmm MD and MT? 17:32 hmmmm isn't MD the debug one 17:33 Krock they eliminate ech other 17:33 Krock looks like MD must be removed 17:33 hmmmm yes 17:34 hmmmm we should be using the static version everywhere 17:34 hmmmm don't get why it's not the case... 17:34 Krock is it because json is included before changing the variable? 17:35 hmmmm there's no way 17:35 hmmmm this is supposed to be run first 17:36 hmmmm and it's not because all the add_subdirectory() calls are before the variables get set 17:36 hmmmm is it supposed to work like this??? 17:42 Krock Looking at http://www.cplusplus.com/reference/cmath/round/ is the change in myround() required to make it work exactly like the round() function 17:42 hmmmm i tested it and what myround() had at first worked good enough i figured 17:45 sfan5 Krock: passing random strings ("/MT") to gcc is guaranteed to break 17:45 sfan5 (in response to https://github.com/minetest/minetest/pull/2657#discussion_r29358189) 17:46 Krock Oh sure, haven't thought this could be a MSVC-only param :3 17:46 sfan5 of couse it is 17:46 sfan5 "/MT" can be a valid path in linux 17:46 sfan5 it can't be a compiler flag 17:47 Krock Ah, that's the reason for '-' and '--' command line args 17:47 sfan5 (what if you want to compile a file called MT at the root /) 17:47 sfan5 additionally /FLAG looks stupid, but thats just a matter of person preference 17:48 est31 mine too btw 17:49 hmmmm ?? 17:49 sfan5 ??? 17:49 hmmmm why add that stuff to DecoSchematic()? 17:50 sfan5 add? you mean remove? 17:50 sfan5 also isn't the parent class constructor automatically called? 17:50 hmmmm yes 17:50 hmmmm so you shouldn't need to explicitly set the same variables set the parent constructor 17:51 sfan5 oh wait,nevermind 17:52 sfan5 umm 17:52 sfan5 Krock: does floor() not work for negative ints in msvc? 17:53 Krock sfan5, it does but what if you get negative numbers to round, as example -5.5? 17:53 sfan5 the old implementation rounds that to -5 17:53 sfan5 why change it 17:53 hmmmm because it's incorrect. 17:53 Krock the orignal round() function rounds it to -6 17:54 Krock and that's already the only reason for that change ^ 17:54 sfan5 i know that it's incorrect, I was asking because the PR didn't mention fixing myround anywhere 17:55 Krock the commit's description is correct. 17:55 sfan5 "numeric.h -> Use - 0.5f for negative numbers" 17:55 sfan5 that doesn't really describe what you changed 17:56 sfan5 imagine reading "numeric.h -> Use - 0.5f for negative numbers" in a commit msg, what do you think the person changed? 17:56 sfan5 you certainly won't come up with the idea that myround was fixed to round correctly when n < 0 18:01 Krock I'm unsure. Is the "parent" initialization function called with the extended DecoSchematic() function? 18:02 hmmmm test it yourself 18:02 Krock yes, that might be the safiest way 18:02 hmmmm hell, even use ideone 18:03 Krock duh. what a great tool! 22:32 hmmmm hrmm krock isn't around 22:33 hmmmm I wonder how Krock was having trouble with his MSVC build when it seems fine on Travis 23:55 est31 hmmmm, travis builds for windows yes but not with visual studio 23:55 est31 but doing a cross build I think 23:55 est31 using clang 23:56 hmmmm oh... well that's not as useful as i thought 23:58 est31 I think what sfan5 did here was quite cool 23:58 est31 or others 23:58 est31 dont know the original author or whose idea it was