Better account.cpp #2145
Better account.cpp #2145
Conversation
christopherlam
force-pushed
the
better-cpp
branch
from
3d2542b to
0cb3a47
Compare
|
Imho the sort can be removed because the splits are already sorted if edit level is zero. |
christopherlam
force-pushed
the
better-cpp
branch
2 times, most recently
from
b06ac03 to
211acd7
Compare
It's sorting
Sorted or not the splits should be grouped so that those belonging to the same transaction are adjacent and that's sufficient for |
But if it's just going to get turned into a
I think that makes the whole comment obsolete... but we should probably introduce a configure check to break the build if anybody tries on a system with 32-bit time_t. |
Dunno... they (i.e. llm) seem to suggest that sorting Transaction* pointers would then lead to sorting by their absolute address. https://claude.ai/share/8d0b3aae-b348-49b4-bb32-eb3297650771 for examples. But I still believe this sorting is unnecessary and even detrimental. The split are definitely ordered by posting date which ensures efficient destruction from the
added in... |
christopherlam
force-pushed
the
better-cpp
branch
from
211acd7 to
b80e57e
Compare
I think most compilers would. "Undefined behavior" just means that the standard doesn't require the compiler to do--or not do--anything in particular. It's allowed to wipe your hard drive if the implementors want, but that would generally be too much trouble.
As written it's definitely unnecessary but since it doesn't do anything it can't be detrimental. If you were to change to actually sort by the
Posting date isn't sufficient, there are often many transactions with the same posting date. But as I said earlier the regular sort algorithm ensures that splits are grouped by transaction (otherwise they wouldn't be immediately adjacent in the register) so this sort probably doesn't do anything. Efficiency is about the memory we're accessing being contiguous and small enough for several items to fit in cache. Neither of those will be true no matter what, and time spent cleaning up memory is insignificant compared to time spent writing to disk so it's not worth worrying about. |
|
Unfortunately Mingw32 still has the problem: But I set a bp on g_date_set_time_t and it didn't fire so maybe GDate has changed. More research required. |
Ok maybe it's minor. I've confirmed that @@ -1598,6 +1598,15 @@ xaccAccountDestroy (Account *acc)
xaccAccountCommitEdit (acc);
}
+static void dump_transactions (const char* header,
+ std::vector<Transaction*> transactions)
+{
+ printf ("%s dump: ", header);
+ for (auto t : transactions)
+ printf ("transaction %p\n", &*t);
+ printf ("\n");
+}
+
void
xaccAccountDestroyAllTransactions(Account *acc)
{
@@ -1607,7 +1616,9 @@ xaccAccountDestroyAllTransactions(Account *acc)
std::transform(priv->splits.begin(), priv->splits.end(),
back_inserter(transactions),
[](auto split) { return split->parent; });
+ dump_transactions ("before sort", transactions);
std::stable_sort(transactions.begin(), transactions.end());
+ dump_transactions ("after sort", transactions);
transactions.erase(std::unique(transactions.begin(), transactions.end()),
transactions.end());
qof_event_suspend();outputs
win32 has 2038 bug? 😨 |
because they are already sorted. besides, std::sort void* pointers should have Comparator because comparing void* is undefined https://eel.is/c++draft/expr.rel#4
avoid malloc/free
instead of vice-versa. - avoids malloc/free in gnc_gdate_set_today - avoids unnecessary dmy->julian->dmy(*) conversion (*) the GDate is typically used by gdate_to_time64 which performs a julian->dmy conversion, and this is now avoided
christopherlam
force-pushed
the
better-cpp
branch
from
b80e57e to
28ae462
Compare
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64?view=msvc-170. Mingw32 seens to turn on |
Ok I've confirmed the bug exists. On windows. xaccTransGetDate has 32bit time_t crash. Test: create an annual sx starting today. Manually change the windows date to 2037. Select Since Last Run to trigger the many sx to be presented for creation. Create them successfully. Now change date to 2039, launch SLR which calls TransSetDate and it crashes. |
That's interesting, xaccTransGetDate shouldn't use time_t at all. Can you get a stack trace? |
Can't seem to, I'm afraid. running gdb causes a python-not-found error on my seldom-used win machine. |
|
Try this: Install drmingw in a MSYS2 or Mingw32 terminal window That should print a stack trace and a bunch of other good stuff to the terminal. |
|
Ok this cmdline was tricky to start. Maybe the following? |
|
another more comprehensive one. latest nightly. date changed to 2055 or something. crash when launching SLR. seems to crash on gnucash/libgnucash/engine/gnc-date.cpp Line 262 in dde1046 |
|
Perfect. Dang, they get the current date-time with They've also changed the exception they throw from their own |
Boost::date_time has changed to throwing a std::out_of_range instead of a boost::date_time::gregorian::bad_year when a date is outside of the 1400-9999 year range it can deal with. We also recently discovered that it will use the system localtime function when creating a new date which can lead to a 2038 failure. Use std::chrono to resolve that problem.
2 participants
std::sortvoid* pointers should have Comparator -- apparently comparing void* is UB https://eel.is/c++draft/expr.rel#4.3 (says chatgpt)gnc_account_foreach_splitforward loops only because the reverse iteration isn't used at all. having thereversearg is an unnecessary complication.