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

libexpr: split yacc prologue & epilogue (NFC) #9059

Closed
wants to merge 1 commit into from

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented Sep 28, 2023

Motivation

This is an NFC PR that splits epilogue & prologue from parser.

As mentioned in #8812, we can add static checking tools & auto
formatting for these files, but if it is written .y directly, the clang
parser cannot understand the code is actually "C++".

Context

#8812
#6721

Priorities

Add 👍 to pull requests you find important.

@edolstra
Copy link
Member

Note that we use .cc, not .cpp, as the extension for C++ files.

What does "NFC" mean in this context?

@inclyc
Copy link
Member Author

inclyc commented Sep 29, 2023

NFC

NFC > > "No Functional Change"

@inclyc
Copy link
Member Author

inclyc commented Sep 29, 2023

Note that we use .cc, not .cpp, as the extension for C++ files.

Thanks for pointing this out, however if use .cc suffix it will be matched by the Makefile rule *.cc and will be built as a standalone source file, which results to ODR violation, maybe I can move these file in directories.

@inclyc inclyc force-pushed the users/inclyc/split-yacc branch from 69df5ef to 6985f96 Compare September 29, 2023 13:00
@Ericson2314
Copy link
Member

I am surprised this works with the *.cc wildcard in local.mk.

@Ericson2314
Copy link
Member

I don't think we should be including .cc files. Can we give it a different extension? I don't really care what it is, hell .hh-noinstall would be fine with me.

@inclyc
Copy link
Member Author

inclyc commented Oct 5, 2023

I don't think we should be including .cc files. Can we give it a different extension? I don't really care what it is, hell .hh-noinstall would be fine with me.

I would like to change the extension to .inc because it indicates this file will be #included, and it contains no header-gaurds (#pragma once)

This is an NFC PR that splits epilogue & prologue from parser.

As mentioned in NixOS#8812, we can add static checking tools & auto
formatting for these files, but if it is written .y directly, the clang
parser cannot understand the code is actually "C++".
@inclyc inclyc force-pushed the users/inclyc/split-yacc branch from 6985f96 to b9f5324 Compare October 5, 2023 02:57
Comment on lines +2 to +3
#ifndef BISON_HEADER
#define BISON_HEADER
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#ifndef BISON_HEADER
#define BISON_HEADER
#pragma once

@@ -0,0 +1,93 @@
#ifdef __clang__
Copy link
Member

Choose a reason for hiding this comment

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

We should really avoid custom extensions like .inc, because that breaks syntax highlighting etc. in most editors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we give it a different extension?

We should really avoid custom extensions like .inc, because that breaks syntax highlighting etc. in most editors.

Would you like to kindly suggest which extension should I use :) ?

Copy link
Member

Choose a reason for hiding this comment

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

@edolstra Yeah the issue is that we don't want the wildcard rules to install this with the other headers with the wildcards that would pick them up.

Besides using a different extension, the only other thing I can think of would be a different directory, e.g. src/*.hh for private headers, and include/*.hh for public headers is pretty convention. But I recall you didn't want that public headers moved away from the source files they go with :).

@inclyc inclyc closed this Dec 29, 2023
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