Skip to content

Support #pragma once and refine codebase #189

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChAoSUnItY
Copy link
Collaborator

@ChAoSUnItY ChAoSUnItY commented Apr 17, 2025

Previously in PR #158, #ifndef is not sufficient enough to address optional file inclusion issue in shecc, since shecc currently includes file before tokenization / parsing pass. Therefore, #pragma once is the only way to prevent recursively file inclusion.

Although #pragma once is not standard anywhere in C standards, major C compilers such as GCC and Clang do support this preprocessor directive.

before #pragma once and codebase refinement:

image

after #pragma once and codebase refinement:

image

Notice for shecc contributors

This is a temporary workaround to enable contributors to have proper intellisense or other LSP suggestions available while developing shecc itself, which expects #pragma once to be removed in shecc source code when new C preprocessor is implemented.

Summary by Bito

This pull request implements `#pragma once` in several files to prevent recursive file inclusion, enhancing the organization of the shecc compiler's codebase. It updates the lexer and parser to effectively handle `#pragma` directives, improving functionality and contributing to a more efficient development experience for contributors.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

@jserv jserv requested review from DrXiao, fennecJ and vacantron and removed request for DrXiao and fennecJ April 17, 2025 08:42
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase the latest master branch.

src/defs.h Outdated
@@ -1,12 +1,12 @@
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move #pragma once right after the license notice.

src/globals.c Outdated
@@ -1,3 +1,4 @@
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit weird to place #pragma once in a non-header file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think current implementation (both MakeFile target script and shecc itself) is capable of linking source file to header file, in other words, we cannot declare global variables and function prototypes in globals.h and include it in other files. Also, even after migrating to header file, the source file would still need to add pragma since again, shecc is not capable of linking which requires to includes source and header file at once. This is workaround should be ignored by gcc when compiling, and is required for shecc in order to not included multiple times.

Previously in PR sysprog21#158, #ifndef is not sufficient enough to address
optional file inclusion issue in shecc, since shecc currently includes
file before tokenization / parsing pass. Therefore, "#pragma once" is
the only way to prevent recursively file inclusion.

Although "#pragma once" is not standard anywhere in C standards, major
C compilers such as GCC and Clang do support this preprocessor
directive.
Copy link
Collaborator

@DrXiao DrXiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed changes look good to me.

Due to the current limitations of shecc, it is still necessary for global.c to use #pragma once to prevent multiple inclusions.

We can resolve this once shecc is improved to handle more complex compilation in the future.
(If necessary, create an issue for tracking.)

Sign up for free to 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.

3 participants