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

[feature]add fmtlib(#2291) #2367

Closed
wants to merge 1 commit into from

Conversation

Tangruilin
Copy link
Contributor

@Tangruilin Tangruilin commented Apr 1, 2023

What problem does this PR solve?

Issue Number: #2291

Problem Summary:

What is changed and how it works?

What's Changed:

add libfmt fot glog

How it Works:
now we can use fmt::print("{}", 4) to format log. Details can look issue#2291

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

"Move-Item -Path support/bazel/WORKSPACE.bazel -Destination WORKSPACE.bazel",
],
)

bind(
name = "braft",
actual = "@com_github_baidu_braft//:braft",
Copy link

Choose a reason for hiding this comment

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

with the code review

The code looks good. There is no obvious bug risk and the patch_cmds/patch_cmds_win are well defined. However, there are a few improvements that can be made.

First, it would be better to use variables instead of hardcoded paths in the patch_cmds and patch_cmds_win. This would make the code more flexible, as different paths could be used in different environments.

Second, the patch_cmds and patch_cmds_win should be moved to a separate file and imported into this file, rather than having all of the commands listed in the same file. This will help keep the code more organized and easier to maintain.

Finally, some comments should be added to the code to explain what each line does and why it is necessary. This will help future developers understand the code more easily.

@Tangruilin
Copy link
Contributor Author

/recheck

@Tangruilin
Copy link
Contributor Author

recheck

@Tangruilin
Copy link
Contributor Author

@wuhongsong please look it. Thanks

@Tangruilin
Copy link
Contributor Author

are there someone review this PR

@fansehep
Copy link
Member

fansehep commented Apr 7, 2023

Can you use fmt in a cpp file? let the build deps have the fmt.

BTW. give a example to how to print a class by libfmt in a cpp file?

@wuhongsong
Copy link
Contributor

@Cyber-SiKu

@@ -37,6 +37,25 @@ git_repository(
commit = "d12de388c97998f5ccd5cb97ed0da728815ef438",
)

git_repository(
name = "fmt",
branch = "master",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a stable version? For example tag: 9.1.0.

@Cyber-SiKu
Copy link
Contributor

Can you use fmt in a cpp file? let the build deps have the fmt.

BTW. give a example to how to print a class by libfmt in a cpp file?

agree! You can modify an old code as an example, or modify some unit tests.

@Cyber-SiKu
Copy link
Contributor

cicheck

1 similar comment
@Cyber-SiKu
Copy link
Contributor

cicheck

@Tangruilin
Copy link
Contributor Author

Can you use fmt in a cpp file? let the build deps have the fmt.

BTW. give a example to how to print a class by libfmt in a cpp file?

ok, i will do it these day

@Cyber-SiKu
Copy link
Contributor

@Tangruilin Will you continue to follow up this PR?

@ilixiaocui
Copy link
Contributor

@Tangruilin Will you continue to follow up this PR?

@Cyber-SiKu
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

cicheck

@Cyber-SiKu
Copy link
Contributor

name = "fmt",
branch = "master",
remote = "https://github.com/fmtlib/fmt",
commit = "bce8d4ed087a0560492426f9f5be2713804daded"
Copy link
Contributor

Choose a reason for hiding this comment

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

miss ,

@Cyber-SiKu
Copy link
Contributor

continue in 2635

@Cyber-SiKu Cyber-SiKu closed this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants