Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactoring] General maintenance #978

Merged
merged 6 commits into from
Jun 14, 2020
Merged

Conversation

Nightwalker-87
Copy link
Member

@Nightwalker-87 Nightwalker-87 commented Jun 4, 2020

  • Reorganised project source directories
  • Grouped source files for win32
  • [refactoring] Build settings for win32
  • Whitespace & code style cleanup
  • Added check for cflag -std=gnu18

(Closes #864, #976) (Fixes #1089)

@Nightwalker-87 Nightwalker-87 force-pushed the project_structure branch 2 times, most recently from b9773d0 to 4cca588 Compare June 4, 2020 19:51
@slyshykO
Copy link
Collaborator

slyshykO commented Jun 4, 2020

I think we should merge getopt mmap mingw folders to win32

@Nightwalker-87 Nightwalker-87 requested a review from slyshykO June 4, 2020 20:02
@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Jun 4, 2020

Good idea. Give me some time, I want to see the CI pass first. I'll let you know as soon as a review makes sense. I just had to fix a mistake.

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Jun 4, 2020

Note: mmap is not only used with win32. It should go to src/ instead.
Also we should not make use of the unistd.h file (it does not seem to have been included in recent versions anyway). I run into conflicting types when it is included. If it works without it, we should scrap it, I think. Can you verify this?

@slyshykO
Copy link
Collaborator

slyshykO commented Jun 4, 2020

unistd.h is used to build with msvc. So I propose to move it to win32/unistd/unistd.h

@slyshykO
Copy link
Collaborator

slyshykO commented Jun 4, 2020

mmap is not only used with win32

if (WIN32 OR MINGW OR MSYS OR MSVC)
    include_directories(src/win32)
    set(STLINK_SOURCE "${STLINK_SOURCE};src/mmap.c;src/win32/mingw.c")
    set(STLINK_HEADERS "${STLINK_HEADERS};src/mmap.h;src/win32/mingw.h")
    if (MSVC)
        include_directories(src/win32/getopt)
        # Use string.h rather than strings.h and disable annoying warnings
        add_definitions(-DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS /wd4710)
    endif ()
endif ()

This code doesn't lie! Only on windows :)

@Nightwalker-87
Copy link
Member Author

This code does not lie either: src/common.c 😒 - Try it out for yourself - whether this is a useful way of implementation - that remains in question.

@Nightwalker-87
Copy link
Member Author

unistd.h is used to build with msvc. So I propose to move it to win32/unistd/unistd.h

We can leave it in src/win32/. I only introduced a subfolder for getopt, because it derives from an external source along with a separate license and readme file.

BTW: Why don't we have it integrated here then:

@slyshykO
Copy link
Collaborator

slyshykO commented Jun 4, 2020

We can leave it in src/win32/

our unistd.h should lay in differ folder than mingw.h cause MinGW contain the custom realisation of unistd.h

@slyshykO
Copy link
Collaborator

slyshykO commented Jun 4, 2020

This code does not lie either: src/common.c 😒 - Try it out for yourself - whether this is a useful way of implementation - that remains in question.

how to move mmap - slyshykO@cacb493

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Jun 5, 2020

This code does not lie either: src/common.c 😒 - Try it out for yourself - whether this is a useful >way of implementation - that remains in question.

how to move mmap - slyshykO@cacb493

Yes, somehow like this. Let me have a closer look first. 😉

- Grouped source files for win32
- Whitespace & code style cleanup
@Nightwalker-87 Nightwalker-87 changed the title Reorganised project source directories [refactoring] Reorganised project source directories Jun 5, 2020
@Nightwalker-87 Nightwalker-87 changed the title [refactoring] Reorganised project source directories [refactoring] General maintenance Jun 5, 2020
@Nightwalker-87 Nightwalker-87 linked an issue Jun 6, 2020 that may be closed by this pull request
@slyshykO
Copy link
Collaborator

slyshykO commented Jun 8, 2020

I'd prefer that all conditions were in brackets even if it contains only one line.

prefer

if( x ) {
    printf("%s", "Hello \n");
}

over

if ( x ) printf("%s", "Hello \n");

And it is strange for me that all return values covered in parentheses.
Is it added something to code security?

@Nightwalker-87
Copy link
Member Author

Nightwalker-87 commented Jun 8, 2020

Yeah, we can set up brackets for if-statements in general. The return statement can either be with or without brackets as far as i can see, but not only if the value is negative - I'd need to look that up.

@Nightwalker-87 Nightwalker-87 force-pushed the project_structure branch 2 times, most recently from 15e57a7 to f4e9187 Compare June 8, 2020 22:54
- Added config for uncrustify style settings
- Applied changes to source files
- Moved some header files
@Nightwalker-87
Copy link
Member Author

@chenguokai & @slyshykO you may review now.
I'm not planning to add anything else.
This PR should introduce a somehow common and unified code appearance and style.
Any remaining leftovers may be corrected later in time should any be found.

@Nightwalker-87 Nightwalker-87 merged commit e506d33 into develop Jun 14, 2020
@Nightwalker-87 Nightwalker-87 deleted the project_structure branch June 14, 2020 15:14
@stlink-org stlink-org locked as resolved and limited conversation to collaborators Jun 29, 2020
@Nightwalker-87 Nightwalker-87 linked an issue Mar 10, 2021 that may be closed by this pull request
@Nightwalker-87 Nightwalker-87 linked an issue Dec 22, 2022 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
3 participants