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

[fix] Add ENABLE_PRINTF switch to control logging information #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zhoujingya
Copy link

#11

@Jules-Kong Jules-Kong requested review from Jules-Kong, yangzexia and yangkex and removed request for Jules-Kong September 26, 2023 07:31
"to the %ld KiB page size: [0x%llX, 0x%llX]\n",
base0, base0 + size0 - 1, long(PGSIZE / 1024), base, base + size - 1);
base0, base0 + size0 - 1, long(PGSIZE / 1024), base, base + size - 1);)

Choose a reason for hiding this comment

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

Put ; out of DEBUG() macro.

Copy link

@dodohack dodohack left a comment

Choose a reason for hiding this comment

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

Fix coding style.
Also the commit message prefix [fix] is not a proper module name or functionality.

config.h.in Outdated

#ifdef ENABLE_PRINTF
#undef DEBUG
#define DEBUG(x) x;

Choose a reason for hiding this comment

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

Don't add ; to this defined macro.

configure Outdated
# Check enviroment virable ENABLE_PRINTF
if [ "$ENABLE_PRINTF" = "ON" ]; then
CXXFLAGS+=" -DENABLE_PRINTF=ON "
fi

Choose a reason for hiding this comment

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

configure file is an auto-generated file, upstream files should be updated as well.

Copy link
Author

Choose a reason for hiding this comment

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

configure file is an auto-generated file, upstream files should be updated as well.

It is not an auto-generated file, run this script to generate makefile

Copy link

@dodohack dodohack Sep 27, 2023

Choose a reason for hiding this comment

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

It is not an auto-generated file, run this script to generate makefile

configure is auto-generated from configure.ac, you can ref to GNU autoconf manual for detail.

Copy link
Author

@zhoujingya zhoujingya Sep 27, 2023

Choose a reason for hiding this comment

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

It is not an auto-generated file, run this script to generate makefile

configure is auto-generated from configure.ac, you can ref to GNU autoconf manual for detail.

I compared the autoconf genarated file with configure, the authour made many changes in configure file, so I think it's ok to do some change in configure file not in configure.ac

@@ -298,7 +298,7 @@ int spike_device::run(meta_data* knl_data,uint64_t knl_start_pc){
uint64_t knlbase=knl_data->metaDataBaseAddr;

if ((ldssize)>0x10000000) {
fprintf(stderr, "lds size is too large. please modify VBASEADDR");
DEBUG(fprintf(stderr, "lds size is too large. please modify VBASEADDR");)

Choose a reason for hiding this comment

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

Indent

Copy link
Author

Choose a reason for hiding this comment

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

Indent

The program's coding style maybe later needed to be modified,I think the maintainer is supposed to create a PR to do this, I will later modify what I do in this PR

@yangzexia yangzexia self-requested a review September 28, 2023 09:12
@Jules-Kong
Copy link
Contributor

please merge two commits into one.

@yangzexia yangzexia linked an issue Dec 7, 2023 that may be closed by this pull request
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.

[log] Log信息太过琐碎的问题
4 participants