[Buildroot] [PATCH 1/6] barebox: fix coding style
Fabio Porcedda
fabio.porcedda at gmail.com
Thu Mar 13 16:35:35 UTC 2014
Hi Arnout and Yann,
thanks for reviewing the patch.
On Wed, Mar 12, 2014 at 7:45 AM, Arnout Vandecappelle <arnout at mind.be> wrote:
> On 03/11/14 18:33, Yann E. MORIN wrote:
>> Fabio, All,
>>
>> On 2014-03-11 13:17 +0100, Fabio Porcedda spake thusly:
>>> Break long lines.
>> [--SNIP--]
>>> -BAREBOX_MAKE_FLAGS = ARCH=$(BAREBOX_ARCH) CROSS_COMPILE="$(CCACHE) $(TARGET_CROSS)"
>>> +BAREBOX_MAKE_FLAGS = ARCH=$(BAREBOX_ARCH) CROSS_COMPILE="$(CCACHE) \
>>> + $(TARGET_CROSS)"
>
> Splitting a line between quotes is evil.
It's a matter of code style because it works fine, doesn't it?
>>
>> Although we have no written rule about thus, I'd rather that folded-lines
>> assignments continue after the '=' sign, like:
>>
>> BAREBOX_MAKE_FLAGS = ARCH=$(BAREBOX_ARCH) CROSS_COMPILE="$(CCACHE) \
>> $(TARGET_CROSS)"
>>
>> It makes it easier to see the assignment.
I don't have a strong opinion about this, i'd like also to be
consistent to the style used in buildroot and i was unable to find any
code that follow that rule.
> Or better yet (and this is the unwritten rule that we tend to follow):
>
> BAREBOX_MAKE_FLAGS = \
> ARCH=$(BAREBOX_ARCH) \
> CROSS_COMPILE="$(CCACHE) $(TARGET_CROSS)"
It's nice, i will do like that.
>
>>
>>> ifeq ($(BR2_TARGET_BAREBOX_USE_DEFCONFIG),y)
>>> -BAREBOX_SOURCE_CONFIG = $(@D)/arch/$(BAREBOX_ARCH)/configs/$(call qstrip,$(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig
>>> +BAREBOX_SOURCE_CONFIG = $(@D)/arch/$(BAREBOX_ARCH)/configs/$(call qstrip,\
>>> + $(BR2_TARGET_BAREBOX_BOARD_DEFCONFIG))_defconfig
>
> Splitting a single path is also evil. We do usually keep the long lines
> in such a case.
This time too it's a matter of code style, isn't it?
>
>>
>> Ditto.
>>
>>> else ifeq ($(BR2_TARGET_BAREBOX_USE_CUSTOM_CONFIG),y)
>>> BAREBOX_SOURCE_CONFIG = $(BR2_TARGET_BAREBOX_CUSTOM_CONFIG_FILE)
>>> endif
>>>
>>> define BAREBOX_CONFIGURE_CMDS
>>> - cp $(BAREBOX_SOURCE_CONFIG) $(@D)/arch/$(BAREBOX_ARCH)/configs/buildroot_defconfig
>>> - $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) buildroot_defconfig
>>> + cp $(BAREBOX_SOURCE_CONFIG) \
>>> + $(@D)/arch/$(BAREBOX_ARCH)/configs/buildroot_defconfig
>>> + $(TARGET_MAKE_ENV) $(MAKE) $(BAREBOX_MAKE_FLAGS) -C $(@D) \
>>> + buildroot_defconfig
>>> endef
>>>
>>> ifeq ($(BR2_TARGET_BAREBOX_BAREBOXENV),y)
>>> @@ -68,7 +73,8 @@ endef
>>> endif
>>>
>>> ifeq ($(BR2_TARGET_BAREBOX_CUSTOM_ENV),y)
>>> -BAREBOX_ENV_NAME = $(notdir $(call qstrip, $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)))
>>> +BAREBOX_ENV_NAME = $(notdir $(call qstrip,\
>>> + $(BR2_TARGET_BAREBOX_CUSTOM_ENV_PATH)))
>
> This one is probably OK :-)
>
>
> Regards,
> Arnout
>
>>
>> Ditto.
>>
>> But I'd like the maintainer to Ack this before you resend. I have no
>> strong opinion about it, I just find it easier to read...
>>
>> Regards,
>> Yann E. MORIN.
Thanks
--
Fabio Porcedda
More information about the buildroot
mailing list