Re: [PATCH] For clang use Blocks instead of nested functions.

[Chih-Hung Hsieh <chh at google dot com>]

As Petr and Mark pointed out, most nested functions in elfutils are small
inline functions that could be replaced with macros. I would be happy to
help
transforming them to macros or file scope static functions.

Please take a look of the new attached
0001-Replace-inline-nested-functions-with-macros.patch,
which converted most nested functions in the src directory.
You can see that luckily clang has "statement expression",
so we can use it for functions with return values.
If that's acceptable to you, we can transform these simple cases first
and then deal with the complicated ones later.

The diff of strip.c is harder to read because the nested "relocate" function
is moved to file scope. The declaration of struct shdr_info is moved too.
The 9 local variables used by "relocate" are all passed as constants with
the help of macro pass_relocate_closure and declare_relocate_closure.
This is probably not the cleanest or fastest transformation,
but it should keep the semantics and easy enough to maintain.
Using so many outer-scope variables is the problem.
Maybe it deserves some refactoring.

Now elfutils is one of the very few packages on Android that requires gcc.
I would like to remove that gcc dependency before Android's gcc become too
old and inferior to clang/llvm like on OS X.

Thanks.


On Fri, Sep 11, 2015 at 5:56 AM, Mark Wielaard <mjw@redhat.com> wrote:

> On Fri, 2015-09-11 at 13:08 +0200, Petr Machata wrote:
> > Having a custom syntax to bridge the differences between LLVM and GCC
> > seems over the top.  It introduces a layer of indirection that is not
> > familiar to C coders, and the usage mode is not immediately obvious
> > (viz the __BLOCK annotations).  It's also fragile--people will forget
> > how to use this and that it's supposed to be used at all, so __BLOCK
> > annotations and variable ordering will go out of sync with
> > requirements.
>
> I agree. I should have said something earlier. But wanted to sleep on it
> a bit before shooting the idea down. Nobody wants to be the bad guy.
> Sorry for not speaking up earlier.
>
> The proposed replacement is clever, but too verbose and has too
> many/duplicate annotations that have to be kept in sync by hand. That
> really destroys the whole reason why we use nested functions in the
> first place.
>
> We use nested functions as a simpler, more (type) safe, way of using
> macros. With the added benefit that they can just use the locally
> lexical scoped variables. So you don't need to mark them all up or pass
> them all through to some static function. It really makes the code much
> less cluttered/readable.
>
> Any proposed replacement should really not make the code much more
> cluttered/verbose. It wouldn't be bad if there is a little preprocessor
> magic used (see for example INTUSE/DEF/DECL). But it shouldn't require
> so much magic that it will be hard to read and maintain/keep up to date
> when changing the code.
>
> Thanks,
>
> Mark
>

As Petr and Mark pointed out, most nested functions in elfutils are small

inline functions that could be replaced with macros. I would be happy to help

transforming them to macros or file scope static functions.

Please take a look of the new attached

0001-Replace-inline-nested-functions-with-macros.patch,

which converted most nested functions in the src directory.

You can see that luckily clang has "statement _expression_",

so we can use it for functions with return values.

If that's acceptable to you, we can transform these simple cases first

and then deal with the complicated ones later.

The diff of strip.c is harder to read because the nested "relocate" function

is moved to file scope. The declaration of struct shdr_info is moved too.

The 9 local variables used by "relocate" are all passed as constants with

the help of macro pass_relocate_closure and declare_relocate_closure.

This is probably not the cleanest or fastest transformation,

but it should keep the semantics and easy enough to maintain.

Using so many outer-scope variables is the problem.

Maybe it deserves some refactoring.

Now elfutils is one of the very few packages on Android that requires gcc.

I would like to remove that gcc dependency before Android's gcc become too old and inferior to clang/llvm like on OS X.

Thanks.

On Fri, Sep 11, 2015 at 5:56 AM, Mark Wielaard <mjw@redhat.com> wrote:

On Fri, 2015-09-11 at 13:08 +0200, Petr Machata wrote:
> Having a custom syntax to bridge the differences between LLVM and GCC
> seems over the top.  It introduces a layer of indirection that is not
> familiar to C coders, and the usage mode is not immediately obvious
> (viz the __BLOCK annotations).  It's also fragile--people will forget
> how to use this and that it's supposed to be used at all, so __BLOCK
> annotations and variable ordering will go out of sync with
> requirements.

I agree. I should have said something earlier. But wanted to sleep on it
a bit before shooting the idea down. Nobody wants to be the bad guy.
Sorry for not speaking up earlier.

The proposed replacement is clever, but too verbose and has too
many/duplicate annotations that have to be kept in sync by hand. That
really destroys the whole reason why we use nested functions in the
first place.

We use nested functions as a simpler, more (type) safe, way of using
macros. With the added benefit that they can just use the locally
lexical scoped variables. So you don't need to mark them all up or pass
them all through to some static function. It really makes the code much
less cluttered/readable.

Any proposed replacement should really not make the code much more
cluttered/verbose. It wouldn't be bad if there is a little preprocessor
magic used (see for example INTUSE/DEF/DECL). But it shouldn't require
so much magic that it will be hard to read and maintain/keep up to date
when changing the code.

Thanks,

Mark

Attachment: 0001-Replace-inline-nested-functions-with-macros.patch
Description: Binary data