Bug 68209 - [EFL] Change WebKit EFL coding style
Summary: [EFL] Change WebKit EFL coding style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 68231 68321 68867 69073 69988 70883 71433 74474
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-15 19:16 PDT by Gyuyoung Kim
Modified: 2011-12-29 19:36 PST (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-09-15 19:16:05 PDT
WebKit EFL port has used EFL coding style. However, EFL port has used C-coding style despite files are C++. In addition, this style disturbed reviewers to review EFL patches. So, we discussed this issues by means of webkit-efl mailing list.  https://lists.webkit.org/pipermail/webkit-efl/2011-September/000049.html 

I list current issues up as below. And, this bug is a meta bug to track this issues.

1. pointer operator coding style based on efl coding style.
   : pointer('*') operator coding style follows efl coding style. It looks we need to adhere webkit coding style.

2. Should we use EINA_TRUE and EINA_FALSE?
   : It can be replace with true / false.  

3. Internal functions are using '(void)' parameter.
   : As we already discussed, internal function needs to use C++ coding style.

4. Manual memory management.
   : ewk_xxx files are using malloc, calloc and realloc instead of smart pointer.
      For example, GOwnPtr.

5. C style casting operator.
   : Though file format is .cpp, some codes are still using c type casting operator.
	
6. Meaningless parameter name.
   : We have used coding style of efl parameter so far. However, that is not webkit coding style.
      I heard that port implementation tends to be platform coding style. So, I added a parameter coding style exception to 
      style checker for efl port. But, now I'm not sure whether we're able to use it continually.
Comment 1 Lucas De Marchi 2011-09-17 09:08:50 PDT
(In reply to comment #0)
> WebKit EFL port has used EFL coding style. However, EFL port has used C-coding style despite files are C++. In addition, this style disturbed reviewers to review EFL patches. So, we discussed this issues by means of webkit-efl mailing list.  https://lists.webkit.org/pipermail/webkit-efl/2011-September/000049.html 
> 
> I list current issues up as below. And, this bug is a meta bug to track this issues.
> 
> 1. pointer operator coding style based on efl coding style.
>    : pointer('*') operator coding style follows efl coding style. It looks we need to adhere webkit coding style.

Ok, on bug 68231 I sent an initial configuration for uncrustify to fix this.

> 
> 2. Should we use EINA_TRUE and EINA_FALSE?
>    : It can be replace with true / false.

The problem here is that in public functions we must use Eina_Bool type. It's somewhat automatic then in those functions to use EINA_TRUE/EINA_FALSE.

Internal functions should be fine using bool and thus true/false. For public functions, I'm not sure what approach we should take.

> 3. Internal functions are using '(void)' parameter.
>    : As we already discussed, internal function needs to use C++ coding style.

Agreed.

> 4. Manual memory management.
>    : ewk_xxx files are using malloc, calloc and realloc instead of smart pointer.
>       For example, GOwnPtr.

These are more difficult to handle. Recently Raphael Kubo converted some of them.


> 5. C style casting operator.
>    : Though file format is .cpp, some codes are still using c type casting operator.

This is another easy one. Since most of the time we are converting C structs or native types, almost all of them will be converted to static_cast<>. We could even automatically convert all of them to static_cast<> and going back if we need another type of cast.


> 6. Meaningless parameter name.
>    : We have used coding style of efl parameter so far. However, that is not webkit coding style.
>       I heard that port implementation tends to be platform coding style. So, I added a parameter coding style exception to 
>       style checker for efl port. But, now I'm not sure whether we're able to use it continually.

Some of them are common language to EFL committers/users. I don't think changing it is a good idea, but if it's required we can use sed/coccinelle for this task.
Comment 2 Kenneth Rohde Christiansen 2011-09-17 10:41:08 PDT
> > 2. Should we use EINA_TRUE and EINA_FALSE?
> >    : It can be replace with true / false.
> 
> The problem here is that in public functions we must use Eina_Bool type. It's somewhat automatic then in those functions to use EINA_TRUE/EINA_FALSE.

For return values this should be fine.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-09-17 12:11:27 PDT
(In reply to comment #2)
> > > 2. Should we use EINA_TRUE and EINA_FALSE?
> > >    : It can be replace with true / false.
> > 
> > The problem here is that in public functions we must use Eina_Bool type. It's somewhat automatic then in those functions to use EINA_TRUE/EINA_FALSE.
> 
> For return values this should be fine.

Eina_Bool Foo::bar()
{
    // ...
    return true;
}

should work just fine too, there's just an implicit bool -> unsigned char conversion involved.
Comment 4 Gyuyoung Kim 2011-09-18 21:12:32 PDT
(In reply to comment #3)

> Eina_Bool Foo::bar()
> {
>     // ...
>     return true;
> }
> 

Can this conversion to reduce readability or make confusion to ewk contributor ? I think it is better to use only EINA_BOOL in public functions.
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-12-01 05:13:41 PST
All the bugs this report depends on have been closed, can we close this one too?
Comment 6 Gyuyoung Kim 2011-12-13 20:32:15 PST
(In reply to comment #5)
> All the bugs this report depends on have been closed, can we close this one too?

I'd like to close this bug after Bug 74474 is landed. Because, that bug is also similar bug.
Comment 7 Gyuyoung Kim 2011-12-14 22:40:01 PST
All coding style issues mentioned by this bug are closed. So, I close this bug.