Skip to content

Commit

Permalink
ActiveAE: initialize engine stats before first usage, credits to jimc…
Browse files Browse the repository at this point in the history 
…arroll and Valgrind
  • Loading branch information
@FernetMenta
FernetMenta committed Sep 18, 2014
1 parent f6a3308 commit 83d9b05
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions xbmc/cores/AudioEngine/Engines/ActiveAE/ActiveAE.cpp
View file
Expand Up @@ -175,6 +175,7 @@ CActiveAE::CActiveAE() :
m_audioCallback = NULL;
m_vizInitialized = false;
m_sinkHasVolume = false;
m_stats.Reset(44100);
}

CActiveAE::~CActiveAE()
Expand Down

15 comments on commit 83d9b05

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, there are some more: https://gist.githubusercontent.com/ace20022/f04296356a3311eb3423/raw/2163028471fb5ef2828bd6091c0ed29284679ca1/AE.txt

@FernetMenta
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe but 99% are false positives. do you know of any true positives?

@jimfcarroll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just the output of a source code analyzer. Not a memory debugger/profiler. They're all "true positivies" :-) ... everywhere it says you don't initialize a value in a constructor, you don't . :-)

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Albeit, it's not saying that you use it uninitialized nor does it check if you use a method to do so, iirc. But there're also some performance messages, that might worth a look when someone finds some time

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My post was not intended to blame someone!

@jimfcarroll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ... it just makes comments on style and convention. The Valgrind output probably does contain many false positives. I'm going to try it again with this change when I get a chance.

@fritsch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was quite gracious on the AESinkPULSE. It does not like the c casts which we do in many different places. Not sure if it would really improve something to add static_cast<type*>(p) instead - do we want that?

@fritsch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that: fritsch@264eb34
I used static_casts to really keep the ptr adress the void+ points to. That way we have at least compile time checking.

@FernetMenta
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, if we intend to use those kind of tools more frequently and want to rely on the output, we should agree on some coding styles.
At this time I am undecided if this would be a good idea or not. If devs a forced to initialize a member does not mean that this value makes much sense.

@fritsch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I second that one. Good example is the last entry:

[Utils\AEStreamInfo.cpp:66]: (warning) Member variable 'CAEStreamInfo::m_buffer' is not initialized in the constructor.

That's an array on the stack. memsetting it to zero won't be worth a single thing.

The c-style casts are pure warnings (though the static_casts) introduce more safety concerning "hacks".

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normaly I supress the c-warnings. The tool should simply be a help. Unit members may cause issues, but ofc don't have to. I guess no one disagrees on the static or incr. messages for example.

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. This http://www.agner.org/optimize/ is quite nice to browse

@FernetMenta
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this topic could be an interesting discussion for devcon.

@ace20022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be blood :P

@fritsch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so :-)

Please sign in to comment.