Skip to content

Conversation

@per1234
Copy link
Contributor

build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards:

<command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name <command-line>:0:14: warning: ISO C99 requires whitespace after the macro name 

I have followed the convention of the other build.board names by replacing - with _.

Related:
#812
#1187

build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards: <command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name <command-line>:0:14: warning: ISO C99 requires whitespace after the macro name I have followed the convention of the other build.board names by replacing - with _.
@per1234per1234 changed the title Convert - to _ in build.board namesChange - to _ in build.board namesApr 20, 2018
@MCUdude
Copy link

YES, finally! Downloaded and installed arduino-esp32 last week, and the list of compile warnings was just overwhelming!

@reaper7
Copy link
Contributor

reaper7 commented Apr 21, 2018

I think that also variant name and corresponding to variant subfolder
should be changed from names with - to _
as is done for m5stack #1242

@per1234
Copy link
ContributorAuthor

the list of compile warnings was just overwhelming!

@MCUdude it seems there is the intention to avoid warnings in this project because they actually use -Werror=all when warnings are set to More or All in File > Preferences. That evidently doesn't upgrade preprocessor warnings to errors. The side effect of this is that warning level preference can affects whether code will compile. That's been the source of some confusion but it's for a good cause.

I think that also variant name and corresponding to variant subfolder
should be changed

I'm happy to do so if that's what the team wants.

The current build.variant value works fine since it determines a folder name and folder names may contain -, no problem. Changing the variant name was discussed in #812 and in that case it was decided that the build.variant value should not be changed and that decision has stood to this day.

The benefit of changing build.variant is purely for the sake of consistency with build.board but I really don't see that as a big thing since these two properties are used for completely different purposes.

The downside to the change is that it's harder to follow Git history through folder name changes. However, these variants only have one or two commits each so that's much of a problem. Certainly it's better to do it now than later.

@per1234
Copy link
ContributorAuthor

There is another possible downside to the variant name change I just thought of. It's possible to reference variants from another package (e.g. myboard.build.variant=espressif:esp32-gateway). So you could consider altering the name of a variant to be a breaking API change.

A Google search didn't find any packages that reference these variants so that breakage may be purely theoretical.

@reaper7
Copy link
Contributor

it's just a suggestion...
You convinced me that it was pointless :)

@me-no-devme-no-dev merged commit 25678f4 into espressif:masterMay 14, 2018
Curclamas pushed a commit to Curclamas/arduino-esp32 that referenced this pull request Aug 21, 2018
build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards: <command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name <command-line>:0:14: warning: ISO C99 requires whitespace after the macro name I have followed the convention of the other build.board names by replacing - with _.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@per1234@MCUdude@reaper7@me-no-dev