Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure LogDataWorkerThread::interruptRequested_ atomicity. #220

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gin-ahirsch
Copy link
Contributor

Includes a couple of refactorings as separate commits.

@gin-ahirsch gin-ahirsch force-pushed the logdataworkerthread branch 2 times, most recently from 5a54dd6 to 2cbafe9 Compare June 15, 2018 13:49
variar added a commit to variar/klogg that referenced this pull request Jun 16, 2018
@nickbnf
Copy link
Owner

nickbnf commented Jun 18, 2018

Aren't maxLength_ and indexedSize_ uninitialised?

@z33ky
Copy link

z33ky commented Jun 18, 2018

Members not listed in the initializer list of a constructor are default-initialized before the body runs and scalars are default-initialized to 0.

@variar
Copy link
Contributor

variar commented Jun 18, 2018

Scalars are initialized to 0 during zero-initialization that is done for members of value-initialized class types that have no constructors. For IndexingData class there is user-provided constructor that does not have maxLength_ and indexedSize_ the initializer list, so default initialization should be performed and these members should end up to have indeterminate value. I think adding them to class initializer list is safer.

@nickbnf
Copy link
Owner

nickbnf commented Jun 18, 2018

@z33ky, do you have a source?

@variar, yes this is my recollection as well, in that case, there is a non-default ctor, so zero-initialisation won't be done.

The simple fact we have to discuss this is a strong argument for explicitly initialising everything anyway :-)

@z33ky
Copy link

z33ky commented Jun 18, 2018

@nickbnf

Using the C++11 standard draft n3337 (from 2012-01-16)

12.6 Initialization [class.init]

§1

When no initializer is specified for an object of (possibly cv-qualified) class type (or array thereof), or the initializer has the form (), the object is initialized as specified in 8.5.

8.5 Initializers [dcl.init]

--edit--
§ 11

If no initializer is specified for an object, the object is default-initialized; if no initialization is performed, an object with automatic or dynamic storage duration has indeterminate value.

§ 6

To default-initialize an object of type T means:

  • if T is a (possibly cv-qualified) class type [...]
  • if T is an array type [...]
  • otherwise, no initialization is performed.

You seem to be correct, zero-initialization is not performed.

--edit end--

§7

To value-initialize an object of type T means:

  • if T is a (possibly cv-qualified) class type [...]
  • if T is a (possibly cv-qualified) non-union class type [...]
  • if T is an array type [...]
  • otherwise, the object is zero-initialized.

§3

To zero-initialize an object or reference of type T means:

  • if T is a scalar type (3.9), the object is set to the value 0 (zero), taken as an integral constant expression, converted to T

@nickbnf
Copy link
Owner

nickbnf commented Jun 18, 2018

Thanks @z33ky. Sorry but I still don't see how this apply to our case. maxLength_ and indexedSize_, in this patch, don't have any initializer so §10 doesn't apply here. However:

§11

If no initializer is specified for an object, the object is default-initialized; if no initialization is performed, an object with automatic or dynamic storage duration has indeterminate value. [ Note: Objects with static or thread storage duration are zero-initialized, see 3.6.2. — end note ]

So those two variables will be default-initialized (as opposed to value-initiliazed).

Then:
§6

To default-initialize an object of type T means:
— if T is a (possibly cv-qualified) class type (Clause 9), the default constructor for T is called (and the initialization is ill-formed if T has no accessible default constructor);
— if T is an array type, each element is default-initialized;
— otherwise, no initialization is performed.
If a program calls for the default initialization of an object of a const-qualified type T, T shall be a class type with a user-provided default constructor.

Given those two variables are neither classes or array, surely they are not initialised are they?

@z33ky
Copy link

z33ky commented Jun 19, 2018

Yes, just after posting I realized this and edited the message.

You seem to be correct, zero-initialization is not performed.

@z33ky
Copy link

z33ky commented Jun 19, 2018

Also I haven't yet mentioned that gin-ahirsch is my account from work. I'll be fixing the PR on Thursday.

@gin-ahirsch gin-ahirsch force-pushed the logdataworkerthread branch from 2cbafe9 to 0296b42 Compare June 21, 2018 08:10
@gin-ahirsch
Copy link
Contributor Author

Rebased with

index bf33169..5312d44 100644
--- a/src/data/logdataworkerthread.h
+++ b/src/data/logdataworkerthread.h
@@ -36,8 +36,6 @@
 class IndexingData
 {
   public:
-    IndexingData() { }
-
     // Get the total indexed size
     qint64 getSize() const;
 
@@ -67,8 +65,8 @@ class IndexingData
     mutable QMutex dataMutex_;
 
     LinePositionArray linePosition_;
-    int maxLength_;
-    qint64 indexedSize_;
+    int maxLength_{0};
+    qint64 indexedSize_{0};
 
     EncodingSpeculator::Encoding encoding_{EncodingSpeculator::Encoding::ASCII7};
 };

danberindei pushed a commit to danberindei/glogg that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants