Tencent / flare

Flare是广泛投产于腾讯广告后台的现代化C++开发框架,包含了基础库、RPC、各种客户端等。主要特点为易用性强、长尾延迟低。
Other
1.33k stars 200 forks source link

fix(logging): FormatLog use perfect forwarding #116

Closed 4kangjc closed 1 year ago

4kangjc commented 1 year ago

https://github.com/Tencent/flare/blob/16f2488b9fe3b730b690669fb088ab60862e4bf5/flare/base/internal/logging.h#L397-L398

虽然这个是万能引用的缺点,但感觉这样处理还是不太好?

0x804d8000 commented 1 year ago

感觉相对于老的实现好像没解决什么实际的use case 啊

另外这个我感觉对编译速度和理解难度都有一定的影响,如果没有实际的case的话建议还是keep it simple & stupid

4kangjc commented 1 year ago

感觉相对于老的实现好像没解决什么实际的use case 啊

另外这个我感觉对编译速度和理解难度都有一定的影响,如果没有实际的case的话建议还是keep it simple & stupid

嗯嗯

0x804d8000 commented 1 year ago

https://github.com/Tencent/flare/blob/16f2488b9fe3b730b690669fb088ab60862e4bf5/flare/base/internal/logging.h#L397-L398

虽然这个是万能引用的缺点,但感觉这样处理还是不太好?

两害相权取其轻了,转发引用好像没啥必要,反而碰到bit field会给用户一屏幕迷一样的错误信息

4kangjc commented 1 year ago

OK, 但其实我觉得文档里说清楚就行, 遵守fmt::format的接口(forward)可能会更好一点, 对于左值应该没啥影响, 右值可以直接move?

4kangjc commented 1 year ago

OK, 但其实我觉得文档里说清楚就行, 遵守fmt::format的接口(forward)可能会更好一点, 对于左值应该没啥影响, 右值可以直接move?

文档里说清楚那几种情况不能写,比如bit field, 说明解决方法, 比如用'+'

4kangjc commented 1 year ago

OK, 但其实我觉得文档里说清楚就行, 遵守fmt::format的接口(forward)可能会更好一点, 对于左值应该没啥影响, 右值可以直接move?

文档里说清楚那几种情况不能写,比如bit field, 说明解决方法, 比如用'+'

还有指针之类的, 这个const-ref也不能解决吧

0x804d8000 commented 1 year ago

OK, 但其实我觉得文档里说清楚就行, 遵守fmt::format的接口(forward)可能会更好一点, 对于左值应该没啥影响, 右值可以直接move?

OK, 但其实我觉得文档里说清楚就行, 遵守fmt::format的接口(forward)可能会更好一点, 对于左值应该没啥影响, 右值可以直接move?

文档里说清楚那几种情况不能写,比如bit field, 说明解决方法, 比如用'+'

还有指针之类的, 这个const-ref也不能解决吧

这个不影响,不过目前指针始终得转成void*

0x804d8000 commented 1 year ago

关于+x.field,当年的话这个还是比较难懂的。不过现在随着std::format更大范围的使用,用户应该已经习惯于这个问题了,可能改成forwarding reference也是可以的

4kangjc commented 1 year ago

https://github.com/CnTransGroup/EffectiveModernCppChinese/blob/master/src/5.RRefMovSemPerfForw/item30.md 在logging的文档里说明一下, 再格外附上指针之类的就蛮好的 总之就是概括一下失败的哪几种情况, 说明一下解决办法

4kangjc commented 1 year ago

看上去是waitable_test里的ConditionVariableNoTimeout把fiber跑丢了

0x804d8000 commented 1 year ago

一直没时间看那个测试,跟这个改动无关,先merge了🙏

4kangjc commented 1 year ago

一直没时间看那个测试,跟这个改动无关,先merge了🙏

OK. 你有时间的时候看看