Windows fixes #1537
Windows fixes #1537
Conversation
We don't need any special file organization, so remove them to ease maintenance.
Fix bug in tracon table debug when USE_TABLE_TRACON is disabled, introduced in commit 2112df2
The value of the w_count_bin type argument (c) is already clamed, so the table value should be the same. But using the count_bin type value (*e) avoids a warning of possible loss of data (64 -32 bit conversion) that is enabled by default on Windows.
(I would like to clean up implicit conversions.)
This prevents MSVC warnings after assert(0, ...) at end of functions that returns a value only at their middle.
There is no need to ensure the "id" variable is incremented in an atomic way, because we don't care if two threads will use the same value. The crash that got debugged on commit 9f73b37 was due to another reason: not using "id" variable if it is 0. This has been fixed. Removing it gets rid of some small overhead, and also allows MSVC compilation.
| static _Atomic(uint16_t) id = 0; | ||
| uint16_t lid = ++id; /* A local copy, for multi-threading support. */ | ||
| static uint16_t id = 0; | ||
| uint16_t lid = ++id; /* A stable local copy, for multi-threading support. */ |
There was a problem hiding this comment.
This will clearly fail when multi-threading, as the increment ++id can occur in two different threads (and so two threads get the same lid) which means that the assert at line 1470 of parse/count.c might trigger. I'm going to put this back, but then disable VERIFY_MATCH_LIST by default.
There was a problem hiding this comment.
Such a problem cannot happen, since each LG thread accesses only its own data structures!
There was a problem hiding this comment.
Note that the lid is put in the element list that a form_match_list() returns, and since it is an auto variable, all the elements of the list are guaranteed to get the same lid. When form_match_list() returns, the thread that has invoked it fetches the id from the first element and validates that the rest use the same id (to validate that the list has not been corrupted, as happens sometimes during development). So the code here doesn't care at all what lid values are used in other threads, and even not in the same thread (as long as lid is constant in a single invocation of form_match_list()`, which is guaranteed). They can be always the same, or any random ones.
The bug that got fixed then was due to an uninitialized id field on id==0.
There was a problem hiding this comment.
OK. I skimmed the code and saw this:
link-grammar/link-grammar/parse/count.c
Line 1472 in c098940
and (without over-analyzing it) made me think that assert might now trigger by accident. So I got nervous about it. So I put the Atomic back in, and undef'ed VERIFY_MATCH_LIST so it becomes invisible.
There was a problem hiding this comment.
BTW, my comment "for multi-threading support" just tells why the assignment to an auto variable lid is needed.
This is because a stable lid value is needed during the current form_match_list() call. (Hence I added "stable" to clarify it further.)
There was a problem hiding this comment.
Go ahead and remove the _Atomic if you are sure. I wasn't going to think too hard about it.
These fixes enable the build:
LinkGrammar.vcxproj: Update the list of source file namesform_match_list(): Don't use _AtomicThese fixes fix code that causes warnings:
table_lookup(): On!USE_TABLE_TRACON, return Count_bin typeestimate_log2_table_size(): Avoid warning on conversion to/from doubleChange
GNUC_NORETURNtoNORETURNand define for MSVC tooMSVC: Remove the
vcxproj.filtersfilesGitHub notifies: "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." I don't understand this and hope it is just a GitHub bug.
table_count(): Bugfix !USE_TABLE_TRACON
This is not a bug in the production code, but it happens when defining
USE_TABLE_TRACONasfalsefor debugging. In that case, the function always returns an irrelevant wrong count 1.The rest of the
table_lookup()are improvements I found when fixing the w_Count_bin-related warning.