| Time |
Nick |
Message |
| 00:13 |
|
YuGiOhJCJ joined #minetest-dev |
| 02:23 |
|
ANAND joined #minetest-dev |
| 02:32 |
|
ClobberXD joined #minetest-dev |
| 03:56 |
|
AntumDeluge joined #minetest-dev |
| 04:09 |
|
ssieb joined #minetest-dev |
| 04:50 |
|
ANAND joined #minetest-dev |
| 05:39 |
|
AntumDeluge joined #minetest-dev |
| 06:12 |
|
AntumD joined #minetest-dev |
| 07:31 |
|
karamel joined #minetest-dev |
| 07:48 |
|
AntumD joined #minetest-dev |
| 08:06 |
|
ensonic joined #minetest-dev |
| 08:08 |
|
Krock joined #minetest-dev |
| 08:27 |
Krock |
nerzhul, so shall I add the comments or whitelist the file? for me it looks like you're in favour for the former, but paramat disagrees on that |
| 09:19 |
|
AntumDeluge joined #minetest-dev |
| 09:44 |
|
Beton joined #minetest-dev |
| 10:25 |
|
Fixer joined #minetest-dev |
| 10:35 |
red-001 |
#7484 |
| 10:35 |
ShadowBot |
https://github.com/minetest/minetest/issues/7484 -- Fix buffer overrun in SRP by red-001 |
| 10:36 |
red-001 |
might be a good idea to write unit tests for SRP |
| 10:41 |
|
Krock joined #minetest-dev |
| 11:00 |
Krock |
sfan5, #7482 looks good. Is it tested yet? |
| 11:00 |
ShadowBot |
https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 |
| 11:00 |
sfan5 |
haven't tested yet |
| 11:01 |
Krock |
okay |
| 11:01 |
sfan5 |
if you have the android build stuff installed, it should be quick to test |
| 11:01 |
sfan5 |
otherwise i can do it later today |
| 11:01 |
Krock |
I'd have to set it up first. Testing is after all not much of a thing as soon building works |
| 11:25 |
|
YuGiOhJCJ joined #minetest-dev |
| 13:09 |
|
ClobberXD joined #minetest-dev |
| 13:30 |
|
antims joined #minetest-dev |
| 13:34 |
|
lumberJ joined #minetest-dev |
| 13:44 |
|
entuland joined #minetest-dev |
| 14:24 |
|
Krock joined #minetest-dev |
| 14:36 |
|
Lunatrius joined #minetest-dev |
| 14:59 |
|
p_gimeno joined #minetest-dev |
| 15:05 |
|
Andrej1 joined #minetest-dev |
| 15:07 |
sfan5 |
Krock: tested it now, works. |
| 15:10 |
Krock |
Thanks. LGTM. |
| 15:11 |
red-001 |
anyone have time to look at 7478, it's a one line fix |
| 15:11 |
red-001 |
*? |
| 15:11 |
red-001 |
*7484 |
| 15:14 |
rubenwardy |
#7484 |
| 15:14 |
ShadowBot |
https://github.com/minetest/minetest/issues/7484 -- Fix buffer overrun in SRP by red-001 |
| 15:30 |
|
Andrej1 joined #minetest-dev |
| 15:37 |
|
Gael-de-Sailly joined #minetest-dev |
| 15:49 |
Krock |
red-001, shouldn't that be g_rand_buff[g_rand_idx] instead of &g_rand_buff[g_rand_idx] ? |
| 15:50 |
Krock |
oh. memcpy requires a pointer, by indexing the array it's not a pointer. nvm |
| 15:51 |
sfan5 |
hm would be nice if gcc had a warning for that |
| 15:52 |
sfan5 |
adding an offset onto an array pointer is quite weird, but i can see why gcc doesn't warn about it |
| 15:53 |
Krock |
well, in murmur hash it's the same to calculate the end address |
| 15:53 |
rubenwardy |
isn't p[a] equiv to p + a*sizeof(*p) |
| 15:54 |
sfan5 |
it is |
| 15:55 |
sfan5 |
Krock: what I mean is that if x is known to be an array on either the stack or in .(ro)data, adding something to &x is most certainly a mistake |
| 15:57 |
|
paramat joined #minetest-dev |
| 15:57 |
sfan5 |
why does srp use this rand buf thing anyway? just get randomness on demand |
| 16:05 |
paramat |
i have pre-approval to do a little intuitive reordering of lua_api.txt as described here https://github.com/minetest/minetest/issues/7404#issuecomment-399678558 please state any objections now before i work on it thanks |
| 16:27 |
|
troller joined #minetest-dev |
| 16:43 |
|
Darcidride_ joined #minetest-dev |
| 16:46 |
paramat |
the linter is being a pain again https://github.com/minetest/minetest/pull/7483#issuecomment-399788100 it confuses contributors and makes them think they are making codestyle mistakes |
| 16:47 |
Krock |
Lint is not wrong with most of the suggestions. the old code is just formatted poorly |
| 16:48 |
paramat |
no |
| 16:48 |
paramat |
that's incorrrect |
| 16:49 |
paramat |
it doesn't know MT codestyle at all so much of what it reports is contrary to our codestyle |
| 16:50 |
Krock |
we already had this topic previously. not just once. Lint offers too few parameters to make it work correctly in all cases for our rules |
| 16:50 |
rubenwardy |
most of that in the PR is correct |
| 16:50 |
paramat |
maybe around 50%, varies according to file. in mapgen files it makes about 50 false reports, but that's the worst case |
| 16:50 |
rubenwardy |
the constructor args is questionable |
| 16:51 |
sfan5 |
>(gui::IGUIEditBox *)hovered) |
| 16:51 |
sfan5 |
this looks pretty weird |
| 16:52 |
paramat |
yes, it's understandable it can't know MT codestyle well, i don't expect it to |
| 16:52 |
sfan5 |
no it's not understandable, if your linter doesn't support your code style you switch to a new linter |
| 16:52 |
paramat |
which is why it's debateable it is worth having. it's useful, but also a huge pain |
| 16:53 |
Krock |
We should consider to program our own Linter with AI and blockchain technique /s |
| 16:54 |
|
srifqi joined #minetest-dev |
| 16:54 |
Krock |
well, what options are there which are supported by the dev tools on GitHub? |
| 16:54 |
sfan5 |
anyway, IMO it would be best to fix the code style in that very PR |
| 16:54 |
paramat |
it tends to teach contributers wrong codestyle, and wastes time because we have to then point out where it is wrong |
| 16:54 |
sfan5 |
Krock: github integration does not really matter, it runs on travis anyway |
| 16:56 |
|
ANAND joined #minetest-dev |
| 16:57 |
Krock |
sfan5, in that case it's a question how much we can customize Travis to make use of another tool |
| 17:01 |
Krock |
Updated #7468 |
| 17:01 |
ShadowBot |
https://github.com/minetest/minetest/issues/7468 -- [Don't squash me!] Rename CSM flavours to CSM restrictions by SmallJoker |
| 17:01 |
paramat |
the double-indents it requests are silly |
| 17:01 |
paramat |
1 tab indent for a split line is fine and reduces indentation |
| 17:02 |
paramat |
also of course a reviewer has to review codestyle anyway and would catch most of what linter reports, so it doesn't save time |
| 17:03 |
sfan5 |
>[reviewer] would catch most of what linter reports |
| 17:03 |
sfan5 |
good joke |
| 17:04 |
Krock |
we cannot rely on human control with large PRs, where it's more important that there are no problems/bugs than how the code is formatted |
| 17:04 |
paramat |
i'm looking at the lint report now. it is complaining about formatting we have used for readability, like vertical alignment |
| 17:04 |
sfan5 |
also, 1 tab indent for a split line is not what our code style says |
| 17:04 |
paramat |
overall that report is about 50% correct |
| 17:07 |
paramat |
our codestyle doesn't specify indents, however 1 tab indent is fine |
| 17:07 |
paramat |
more just shifts everything too far in some situations which causes more line splitting |
| 17:08 |
paramat |
code formating for readability is very important, we can't make code less readable just for a bad linter |
| 17:09 |
paramat |
so, i think we need a better one, or none |
| 17:09 |
paramat |
the more recent clang-tidy checks are good though |
| 17:09 |
sfan5 |
two tabs to split lines is what we have everywhere else though |
| 17:09 |
paramat |
no |
| 17:10 |
paramat |
it's a mix |
| 17:10 |
sfan5 |
and it's also part of the rules for conditionals |
| 17:10 |
paramat |
yes, i'm not talking about conditionals |
| 17:10 |
sfan5 |
I know |
| 17:11 |
paramat |
it's needed where differentiation is needed, not everywhere |
| 17:11 |
red-001 |
sfan5, idk maybe someone thought getting the data ever single time was a preformence issue |
| 17:12 |
sfan5 |
red-001: probably, but that's premature optimization |
| 17:12 |
red-001 |
the whole file has a very different coding style to the rest of minetest |
| 17:12 |
red-001 |
feels like I'm reading kernal code |
| 17:13 |
Krock |
maybe because we use the kernel code style guidelines? |
| 17:13 |
paramat |
yes we do, for anyhting not specified in our own guidelines |
| 17:14 |
paramat |
*anything |
| 17:14 |
rubenwardy |
kernel code has way more gotos |
| 17:14 |
rubenwardy |
like, if a function doesn't have at least one goto it's not allowed |
| 17:14 |
red-001 |
some gotos > no gotos |
| 17:15 |
* rubenwardy |
added his first feature to the Linux kernel |
| 17:15 |
rubenwardy |
not mainstream, obviously |
| 17:15 |
sfan5 |
i'd propose writing "indent split lines with 2 tabs" (as it's already done for conditionals) into our style guidelines |
| 17:16 |
red-001 |
the whole code could use some cleanup and unittests |
| 17:16 |
rubenwardy |
red-001: minetest or srp? |
| 17:16 |
rubenwardy |
:) |
| 17:16 |
red-001 |
yes |
| 17:17 |
Krock |
^ inclusive or |
| 17:18 |
red-001 |
the tendency to cast irrlicht stuff generates a lot of warnings from address sanitizer |
| 17:19 |
|
troller joined #minetest-dev |
| 17:22 |
red-001 |
how important is s in SRP anyways? |
| 17:22 |
rubenwardy |
loool |
| 17:22 |
rubenwardy |
iSRP |
| 17:22 |
rubenwardy |
by Minetest [inc] |
| 17:23 |
red-001 |
we did kinda accidently make it easy to guess |
| 17:25 |
paramat |
2 tab indent is needed for conditionals, it's not needed elsewhere and just causes more line splitting |
| 17:26 |
paramat |
i think indenting of long lines should be left to coder judgement, whatever makes it more readable |
| 17:28 |
paramat |
in some situations readability is more imprtant than a hard rule, which is why linters inherently fail |
| 17:42 |
srifqi |
PR 7486 seems to be working. |
| 17:42 |
rubenwardy |
# please |
| 17:42 |
rubenwardy |
#7486 |
| 17:42 |
ShadowBot |
https://github.com/minetest/minetest/issues/7486 -- Software inventorycube by numberZero |
| 17:43 |
srifqi |
Okay. |
| 17:44 |
|
srifqi left #minetest-dev |
| 17:46 |
paramat |
many android bugfixes coming in, please review to prepare for 5.0.0 :) |
| 17:47 |
paramat |
btw, is anyone intending to work on securing builtin against CSM? it's a 5.0.0 blocker |
| 17:54 |
|
Beton joined #minetest-dev |
| 17:54 |
paramat |
probably not, which is understandable :) nerz probably won't do it |
| 18:24 |
|
ssieb joined #minetest-dev |
| 18:33 |
|
Robby joined #minetest-dev |
| 18:59 |
|
ssieb joined #minetest-dev |
| 19:02 |
|
Cornelia joined #minetest-dev |
| 19:08 |
p_gimeno |
<rubenwardy> isn't p[a] equiv to p + a*sizeof(*p) |
| 19:08 |
p_gimeno |
^ strictly speaking, no. It's actually equivalent to *(p + a) |
| 19:11 |
p_gimeno |
where p + 1 is a pointer to the element next to the one p points to, i.e. pointer addition has the sizeof implied, sorta |
| 19:16 |
|
paramat joined #minetest-dev |
| 19:25 |
|
CBugDCoder joined #minetest-dev |
| 19:51 |
|
ensonic joined #minetest-dev |
| 19:55 |
|
Icedream joined #minetest-dev |
| 20:14 |
Shara |
Will merge game#2155 and game#2156 in a bit |
| 20:14 |
ShadowBot |
https://github.com/minetest/minetest_game/issues/2155 -- Add butterflies mod by Ezhh |
| 20:14 |
ShadowBot |
https://github.com/minetest/minetest_game/issues/2156 -- Make hidden fireflies floodable by Ezhh |
| 20:24 |
nerzhul |
hey :) |
| 20:25 |
nerzhul |
the android fixes are waited in 0.4.17 too if they permit to fix that game loading problem :( |
| 20:53 |
paramat |
good point |
| 20:54 |
sfan5 |
#7482 fixes the crashes people have been experiencing |
| 20:54 |
ShadowBot |
https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 |
| 20:57 |
nerzhul |
sfan5, approved |
| 21:02 |
|
wowaname joined #minetest-dev |
| 21:02 |
paramat |
affects all platforms, so do we need a 0.4.17.2 release? :) |
| 21:03 |
Krock |
technically yes, but the only popular affected platform for us is Android |
| 21:04 |
sfan5 |
honestly, wondering why nobody has reported the bug on e.g. rpi yet |
| 21:04 |
sfan5 |
guess mt is too hard to get running on an rpi |
| 21:04 |
Krock |
right, RPI is on ARM too |
| 21:04 |
paramat |
ok |
| 21:05 |
sfan5 |
there are two options: 1) whoever tested it used a compiler that wasn't smart enough to do this optimization 2) nobody has hit this bug |
| 21:05 |
Krock |
Megaf used to run a server on the RPI but I'm not sure how up to date he is with the setup |
| 21:05 |
|
Dumbeldo1 joined #minetest-dev |
| 21:05 |
p_gimeno |
3) nobody has bothered to report it (happens quite often) |
| 21:07 |
Krock |
4) they hit the bug but didn't know why it was crashing (lack of developer tools, knowledge and interest to debug) |
| 21:08 |
paramat |
hmm however, surely we need a release of MT 0.4.17.2 if some android bugfixes are now going to be added to the app? otherwise seems messy |
| 21:27 |
red-001 |
can be an android only release |
| 21:53 |
|
VargaD joined #minetest-dev |
| 21:55 |
paramat |
yeah perhaps :) |
| 22:20 |
|
Beton_ joined #minetest-dev |
| 23:04 |
|
paramat joined #minetest-dev |
| 23:06 |
paramat |
will merge #7482 in 5 mins |
| 23:06 |
ShadowBot |
https://github.com/minetest/minetest/issues/7482 -- Fix alignment issues in MurmurHash by sfan5 |
| 23:10 |
paramat |
going to do a little re-ordering of lua_api.txt now so please don't merge any lua_api.txt PRs until tomorrow morning EU time =) |
| 23:10 |
paramat |
merging |
| 23:12 |
paramat |
done |
