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

Logging: Add fmtlib-based macros #3533

Merged
merged 19 commits into from Mar 24, 2018

Conversation

Projects
None yet
5 participants
@daniellimws
Contributor

daniellimws commented Mar 16, 2018

Continue #3449

Changes made:

  • Logging: Add a new set of logging macros to use fmtlib instead.
  • Citra: Migrate to the new logging macros
  • Config: Migrate to the new logging macros
  • SDL: Migrate to the new logging macros
  • Separating code for customizing multiple backends in #3449 for a different PR.

As variadic templates are required to be defined in the header file itself, Entry related code is moved into common/logging/log.h. Hopefully, such a minor design change is ok.

Once this PR is merged, I will create a PR for the second part of #3449, as well as migrating other components to use this macro.


This change is Reviewable

jroweboy and others added some commits Feb 20, 2018

Logging: Add customizable logging backends and fmtlib based macros
* Change the logging backend to support multiple sinks through the
Backend Interface
* Add a new set of logging macros to use fmtlib instead.
* Qt: Compile as GUI application on windows to make the console hidden by
default. Add filter configuration and a button to open log location.
* SDL: Migrate to the new logging macros
SPSCQueue: Add PopWait
Adds a condition var to SPSCQueue so when a new log is pushed it will
wake the consumer thread that is calling PopWait. This only applies to
to queues with NeedSize=true
Logging: Various logging improvements
* Uses PopWait to reduce the amount of busy waiting if there aren't many
new logs
* Opens the log file as shared on windows, letting other programs read
the logs, but not write to them while citra is running
* Flushes the logs to disk if a log >= error arrives
Logging: Fix fmt errors after rebasing with master
fmt was updated during the clang-format update, which breaks the previous implementation of FmtLogMessage

Changes were:
* Move definition of FmtLogMessage into log.h to use variadic templates as FMT_VARIADIC was removed

To supplement the change above:
* Move Entry and CreateEntry into log.h
* Add LogEntry in backend.cpp
Logging: Remove customizable logging backends
Separate code for customizing logging backends from this branch
@cluezbot

This comment has been minimized.

cluezbot commented Mar 16, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-16T13:00:41Z: 935bcdb...daniellimws:b5532babab080d1aeeba4fddd61abee9c57664ab

@cluezbot

This comment has been minimized.

cluezbot commented Mar 16, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-16T13:19:10Z: 935bcdb...daniellimws:c4f98c1a2e78c7e0aeea7d4fb065ac372b0368b9

@daniellimws

This comment has been minimized.

Contributor

daniellimws commented Mar 16, 2018

Oh noes there's a segfault in the tests. I'll look into them later.

Add check if filter is not a nullptr
Fix segfault and pass tests
@cluezbot

This comment has been minimized.

cluezbot commented Mar 17, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-17T14:56:27Z: 935bcdb...daniellimws:2b4998a122de12850509b4c12430266368430a00

@jroweboy

all in all, looks fine to me. still a little upset about the fmt_variadic stuff though. the point in using that was to avoid creating the entry for logs that were going to be filtered anyway. is there no way to still use that macro? if the answer is no, then whatever, i say merge this :P its not a big deal.

}
void LogEntry(Entry& entry) {
if (!filter->CheckMessage(entry.log_class, entry.log_level))

This comment has been minimized.

@jroweboy

jroweboy Mar 20, 2018

Member

you added a null check for filter in the old logging call but not here as well

template <typename... Args>
void FmtLogMessage(Class log_class, Level log_level, const char* filename, unsigned int line_num,
const char* function, const char* format, const Args&... args) {
Entry entry = CreateEntry(log_class, log_level, filename, line_num, function,

This comment has been minimized.

@jroweboy

jroweboy Mar 20, 2018

Member

i don't like paying for the cost of creating the entry before checking to see if its filtered by the global log level. is there really no way to use some clever way around this? (what was wrong with the fmt_variadic macro again? i still see it in the docs...)

This comment has been minimized.

@daniellimws

daniellimws Mar 20, 2018

Contributor

The problem was that we are using a close to master (way past latest official release) version of fmt, and according to the people there it has been removed. So I was asking in discord, (maybe didn't make this clear), of whether we should rollback the version a bit.

This comment has been minimized.

@daniellimws

daniellimws Mar 20, 2018

Contributor

We can perform the check first, by moving the necessary members from filter (iirc) into log.h

The reason I didnt do so was because it doesn't look good design-wise. So I decided not to unless you guys asked for it.

This comment has been minimized.

@jroweboy

jroweboy Mar 20, 2018

Member

did we try to ask them if they knew of any work arounds for our problem? We want the power of a variadic macro, but with the implementation details tucked away nicely in the cpp file. (I remember spending a lot of time just this and never got very far.)

As for pushing the filter to the log header. meh, i guess if thats the best we can do then we might as well.

This comment has been minimized.

@daniellimws

daniellimws Mar 20, 2018

Contributor

They did told me to use 4.1.0. I can ask them for workarounds but I suppose I would need some reasons to justify why we couldn't use the official release.

@daniellimws

This comment has been minimized.

Contributor

daniellimws commented Mar 20, 2018

There can be two ways to this:

  • Rollback fmt to the official 4.1.0 release
  • I'll figure a clean way to perform the checks before creating Entrys

Definitely solvable so I suggest not merging this yet.

Edit:
I would suggest changing fmt version, since there is official documentation.

Add nullptr check to LogEntry
Also remove explicit comparison with nullptr to make code shorter.
@cluezbot

This comment has been minimized.

cluezbot commented Mar 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-20T06:16:33Z: 935bcdb...daniellimws:ceeb2810fe831fba7ad93d0b99dc4ed10a67ad9d

@wwylele

This comment has been minimized.

Member

wwylele commented Mar 20, 2018

fd79b70
fyi this is the fmt version change commit

@daniellimws

This comment has been minimized.

Contributor

daniellimws commented Mar 20, 2018

ok thanks I'll ask the fmt guys

Place FmtLogMessage's definition in backend.cpp
I decided to overload LogMessage because I don't see a reason to come up with a new function name just for this, but if you guys want me to overload FmtLogMessage instead I'm fine with that.
@cluezbot

This comment has been minimized.

cluezbot commented Mar 21, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-21T15:38:36Z: 935bcdb...daniellimws:c892cea02936e509b73ea9ce8ae9302a0bdc2b4b

@MerryMage

LGTM otherwise

// Define the fmt lib macros
#ifdef _DEBUG
#define NGLOG_TRACE(log_class, fmt, ...) \

This comment has been minimized.

@MerryMage

MerryMage Mar 21, 2018

Member

Prefer: NGLOG_TRACE(log_class, ...)

#ifdef _DEBUG
#define NGLOG_TRACE(log_class, fmt, ...) \
::Log::FmtLogMessage(::Log::Class::log_class, ::Log::Level::Trace, __FILE__, __LINE__, \
__func__, fmt, ##__VA_ARGS__)

This comment has been minimized.

@MerryMage

MerryMage Mar 21, 2018

Member

Prefer: __func__, __VA_ARGS__)

##__VA_ARGS__ is a GCC extension and is not standards-compliant.

Ditto for all the other macros.

This comment has been minimized.

@jroweboy

jroweboy Mar 21, 2018

Member

removing that means we'd need to do something like this for the function instead right?

template <class First, class... Args> 
... __func__, First fmt, const &Args... args) {

other wise we couldn't wrap the args with fmt::make_args while still allowing us to LOG_DEBUG(Frontend, "SomeStringWithNoArgs");

This comment has been minimized.

@daniellimws

daniellimws Mar 21, 2018

Contributor

Probably not, it compiled fine.

This comment has been minimized.

@jroweboy

jroweboy Mar 21, 2018

Member

sure, but does it actually print properly?

This comment has been minimized.

@MerryMage

MerryMage Mar 21, 2018

Member

@jroweboy No, that wouldn't be necessary. fmt is still a string, it's just part of __VA_ARGS__, because dealing with zero-sized __VA_ARGS__s in a standard compliant way is messy. (Until C++2a introduces __VA_OPT__, that is.)

This comment has been minimized.

@daniellimws

daniellimws Mar 21, 2018

Contributor

Sorry for the delay in response but yeah I've tested and it works as intended.

Anyways doesn't really matter since merry responded already.

@cluezbot

This comment has been minimized.

cluezbot commented Mar 21, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-21T16:40:56Z: 935bcdb...daniellimws:e6ee1020e16dc00929519743fb71b522a9deb6bf

Fix clang-format
Oops forgot to check...
@cluezbot

This comment has been minimized.

cluezbot commented Mar 21, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-21T16:45:30Z: 935bcdb...daniellimws:4adbf29b0c72cab8517646fd5303932efcba9fc8

daniellimws added a commit to daniellimws/yuzu that referenced this pull request Mar 22, 2018

Logging: Create logging macros based on fmtlib
Add a new set of logging macros based on fmtlib
Similar but not exactly the same as citra-emu/citra#3533

Citra currently uses a different version of fmt, which does not support FMT_VARIADIC so
make_args is used instead. On the other hand, yuzu uses fmt 4.1.0 which doesn't have make_args yet
so FMT_VARIADIC is used.
Remove dependency chrono
Was included last time due to adding Entry into log.h

Now it is no longer needed but I forgot to remove it last time
@cluezbot

This comment has been minimized.

cluezbot commented Mar 22, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-22T13:42:48Z: 935bcdb...daniellimws:11ce0f91abcaa66bcbdc8423ef322b2fbd614d0b

@MerryMage MerryMage merged commit 174e7e2 into citra-emu:master Mar 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 24, 2018

audio_core: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 24, 2018

audio_core: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 24, 2018

audio_core: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

citra_qt: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

network: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

network: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

web_service: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

input_common: Migrate logging macros
Follow-up of citra-emu#3533

Replace logging to use NGLOG instead of LOG

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

common: Migrate logging macros
Follow-up of citra-emu#3533

Replace logging to use NGLOG instead of LOG

This is significantly larger than the previous ones.

daniellimws added a commit to daniellimws/citra that referenced this pull request Mar 25, 2018

common: Migrate logging macros
Follow-up of citra-emu#3533

Replace logging to use NGLOG instead of LOG

This is significantly larger than the previous ones.

MerryMage added a commit that referenced this pull request Mar 27, 2018

network: Migrate logging macros (#3575)
* network: Migrate logging macros

Follow-up of #3533

Replace prefix of all logging macros from LOG to NGLOG

* Remove hash in format

N00byKing added a commit to N00byKing/citra that referenced this pull request Mar 27, 2018

web_service: Migrate logging macros
Follow-up of citra-emu#3533

Replace prefix of all logging macros from LOG to NGLOG

N00byKing added a commit to N00byKing/yuzu that referenced this pull request Apr 13, 2018

Logging: Create logging macros based on fmtlib
Add a new set of logging macros based on fmtlib
Similar but not exactly the same as citra-emu/citra#3533

Citra currently uses a different version of fmt, which does not support FMT_VARIADIC so
make_args is used instead. On the other hand, yuzu uses fmt 4.1.0 which doesn't have make_args yet
so FMT_VARIADIC is used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment