Remove unused beast::currentTimeMillis() #2345
Conversation
|
Less code to cause a bug, awesome! |
Jenkins Build SummaryBuilt from this commit Built at 20180124 - 00:08:14 Test Results
|
|
Thanks, I suspect that there's even more dead code in src/ripple/beast... this one just stood out because apparently the files in It might be interesting to know which code gets removed by ThinLTO, but I'm not sure if that's an available query. Until then it might be probably worth to cut the Ripple beast back a bit manually if you are concerned about obsolete code. |
| @@ -588,10 +587,6 @@ int run (int argc, char** argv) | |||
| // | |||
| int main (int argc, char** argv) | |||
| { | |||
| // Workaround for Boost.Context / Boost.Coroutine | |||
| // https://svn.boost.org/trac/boost/ticket/10657 | |||
| (void)beast::currentTimeMillis(); | |||
There was a problem hiding this comment.
From the comment, the purpose of this code is to call GetTimeZoneInformation before the first coroutine is launched. The bug is marked as "wontfix". So don't we still need this call?
There was a problem hiding this comment.
That's a great question. It's entirely unclear to me how this call: https:/ripple/rippled/pull/2345/files#diff-0da1f01eb51089298d4839f18d92fa41L41 could have the side effect of loading time zone information. But this would not be the only example of twisted software. How would we determine whether we still need the call? I'd prefer not to leave it in place simply because we're afraid to remove it.
There was a problem hiding this comment.
Seems like a windows only issue...
There was a problem hiding this comment.
Do we know what the failure mode would be? Perhaps @bachase can try building this branch and see if it behaves well for him, since he runs Windows.
There was a problem hiding this comment.
It looks like it happens on the windows _f_time call. The timeptr struct contains timezone information: https://msdn.microsoft.com/en-us/library/z54t9z5f.aspx
I'd prefer to leave this in, even if coro's don't currently call GetTimezoneInformation. If it's added later it would be a PITA to track down the crash.
There was a problem hiding this comment.
I prefer not to leave it in it's current form. If it's a Windows only problem we can make a Windows-only call in the #ifdef _MSC_VER immediately below. With a paragraph comment explaining the justification. But I may need to punt and ask someone else on the team to do that, since I don't run a Windows box.
There was a problem hiding this comment.
I'm not observing a crash on windows with this change when running the unit tests. Using the example code on the boost ticket, I also do not observe a crash with VS 2017 on Windows 10. I do not have other versions of windows available to test. Even though I'm unable to recreate, I'm with @seelabs and would prefer not have this bite us in the future.
I can try more testing to see if I can recreate, but frankly this seems fairly low priority relative to other work. I'm fine with @scottschurr's suggestion of just moving the code inside the existing windows specific startup block.
There was a problem hiding this comment.
Thanks @bachase. I have to agree with your assessment of the priority of this work. I'll make a stab at a Windows only version of the code.
|
Tip commit re-adds the Windows song-and-dance with a paragraph comment. But excludes Unix-like operating systems from involvement. |
scottschurr
force-pushed
the
rm-beast-time
branch
from
a4a8279
to
0e2a026
Compare
scottschurr
added
the
Passed
|
Squashed and marked as passed. |
|
In 0.90.0-b5 |
This change removes some obsolete code identified by @MarkusTeufelberger (thanks!).