[vlc-devel] [Patches] Dash: mostly cosmetics

[Hugo Beauzée-Luyssen]

Hi,

On Tue, Dec 6, 2011 at 1:17 PM, Tobias Güntner <fatbull at web.de> wrote:
> Am 01.12.2011 12:15, schrieb Hugo Beauzée-Luyssen:
>
>> That's exactly what I'm trying to do. However I guess your point here
>> is STL is "our" code, while Qt is not. However the point stays, to me.
>
>
> I was referring to Dash only. Qt and STL are both out of scope. Whether Qt
> does or does not handle exceptions is IMHO irrelevant to our discussion.
>
>
>> We should check for bad_allow, bad_cast if we use references,
>> bad_typeid and ios_base::failure.
>> Saying that I've never encountered a code catching these exceptions
>> would be falacious, but true nevertheless.
>
>
> I'm not saying those exceptions must be caught explicitly. IMHO a generic
> catch(std::exception&) at the point of entry is often sufficient, and I'm
> sure you have encountered *that* before. ;)
>

I can live with that indeed :)

>
>> I'm not saying we should get read of the STL exceptions, just most of
>> those in Dash, that are useless most of the time.
>
>
> Oh, I see. I assumed you wanted to remove all exceptions, including non-Dash
> exceptions.
>

At last we understand each other \o/

>
>> Let's take a code sample :
>>
>> Representation.cpp :
>>
>> SegmentInfo*        Representation::getSegmentInfo          () //...
>> {
>>     if(this->segmentInfo == NULL)
>>         throw ElementNotPresentException();
>>
>>     return this->trickModeType;
>> }
>>
>> BasicCMManager.cpp:
>>
>> try
>>     {
>>         SegmentInfo* info = rep->getSegmentInfo();
>>         //....
>>     }
>>     catch(ElementNotPresentException&e)
>>     {
>>         /*TODO Debug */
>>     }
>>
>> The error is handled twice. One so we can raise, and one to catch the
>> error.
>> It looks clearer to me just to return the pointer, regardless of it's
>> value, and check it just once.
>
>
> I agree exceptions seem to be overkill in this case. But before rewriting
> it, please consider:

That was exactly my point :)

> * Is BasicCMManager::getSegments really the best place to handle this error?
Most of the error handling could be done once and for all after the
parsing occurred.

> * Is returning an empty/incomplete result vector really the best way to
> handle this error?

If you mean returning a newly constructed empty vector, then no, I
don't like that very much. However, if we were to return a pre-built
vector, stored in the appropriate instance, I don't have any issue
returning an empty vector.
As said before, once the file was parsed, we can know if there are
some segments or not, and if there's none, that's an error, so we can
either ignore the representation, of the file if no valid
representation may be found, so if an empty vector is an error,
processing will stop before.

> * Is segmentInfo really optional?

If I understand the standard correctly (§5.4.4), if the Representation
tag doesn't contain any SegmentInfo, the Group.SegmentInfoDefault tag
must be used (and it must be combined with every present SegmentInfo
to compute available information about the segment).

> * Should the error be propagated further?
>

Well it doesn't seem like an error anymore.
I'll have to read the code again for that specific case.

>
>> Again, I'm not saying anything about STL's exceptions. They're not
>> handled for now anyway.
>
>
> IMHO this must definitely be fixed. Not catching bad_alloc (or a base class
> thereof) is like ignoring the return value of malloc, i.e, a bug.
>

I agree.
There's quite a good chance many of the dash errors currently handled
by the code may be better addressed by reading the standard again,
using specified default values.
For the "unrecoverable" errors, I'm more in favor of returning a good
old NULL pointer in the getter (or even an empty vector), but having a
prior check, just after the parse operation, to check the file can be
used.

>
> Regards,
> Tobias
>

Regards,

-- 
Hugo Beauzée-Luyssen