Bug 218381 – [WTF] Coding style issues in MediaTime.h

• Status: NEW
• Component: Web Template Framework

[Philippe Normand]

ERROR: Source/WTF/wtf/MediaTime.h:34:  Alphabetical sorting problem.  [build/include_order] [4]                                                                                                                    
ERROR: Source/WTF/wtf/MediaTime.h:69:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:70:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:75:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:76:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:77:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:78:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:93:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:94:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:95:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:96:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:97:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:98:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:99:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fun
ction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                          ERROR: Source/WTF/wtf/MediaTime.h:100:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fu
nction definition out-of-line.  [build/webcore_export] [4]                        
ERROR: Source/WTF/wtf/MediaTime.h:101:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fu
nction definition out-of-line.  [build/webcore_export] [4]                                                                                                                                                         
ERROR: Source/WTF/wtf/MediaTime.h:109:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fu
nction definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WTF/wtf/MediaTime.h:110:  Inline functions should not be in classes annotated with WTF_EXPORT_PRIVATE. Remove the macro from the class and apply it to each appropriate method, or move the inline fu
nction definition out-of-line.  [build/webcore_export] [4]

[Philippe Normand]

Created attachment 412731 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 412731 [details]
Patch

I think these functions are small and should be inline function. So defining in the header is better. Let's remove WTF_EXPORT_PRIVATE from the class, and annotate some of actually exported functions with WTF_EXPORT_PRIVATE.

[Radar WebKit Bug Importer]

<rdar://problem/71114972>

[Philippe Normand]

Created attachment 414209 [details]
Patch

[Philippe Normand]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 412731 [details]
> Patch
> 
> I think these functions are small and should be inline function. So defining
> in the header is better. Let's remove WTF_EXPORT_PRIVATE from the class, and
> annotate some of actually exported functions with WTF_EXPORT_PRIVATE.

Thanks for the review Yusuke, the patch is updated accordingly, hopefully :)

[Philippe Normand]

Created attachment 414216 [details]
Patch

[Alex Christensen]

Comment on attachment 414216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414216&action=review

> Source/WTF/wtf/MediaTime.h:68
> +    inline MediaTime& operator+=(const MediaTime& rhs) { return *this = *this + rhs; }

I don't think the "inline" keyword is needed here

[Philippe Normand]

Created attachment 414252 [details]
Patch

[Alex Christensen]

Comment on attachment 414252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414252&action=review

> Source/WTF/wtf/MediaTime.h:74
> +    inline bool operator<(const MediaTime& rhs) const { return compare(rhs) == LessThan; }

here either.

> Source/WTF/wtf/MediaTime.h:92
> +    inline bool isValid() const { return m_timeFlags & Valid; }

ditto

> Source/WTF/wtf/MediaTime.h:107
> +    inline const int64_t& timeValue() const { return m_timeValue; }

ditto

[Philippe Normand]

If no inline specifier remains in this patch I'm not sure why Yusuke r-'ed the initial patch.

[Yusuke Suzuki]

(In reply to Philippe Normand from comment #10)
> If no inline specifier remains in this patch I'm not sure why Yusuke r-'ed
> the initial patch.

In C++, inline attribute is not for inlining.
If you define function in header, compiler will decide whether inlines or not regardless of inline attribute is attached or not.

But unless LTO is enabled (WTF / JSC is not LTO enabled for now), function defined in cpp will not be inlined.

So the patch should define function in header, and inline attribute is not necessary.