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.

Sign up for GitHub

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

Jump to bottom

Fix glog check type unmatch in Util.cpp #353

Merged
merged 3 commits into from
Nov 4, 2016
Merged

Fix glog check type unmatch in Util.cpp #353

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87a63d2
Fix glog check type unmatch in Util.cpp
gangliao Nov 4, 2016
bfa460c
Update Util.cpp
gangliao Nov 4, 2016
8cebf1f
Update Util.cpp
gangliao Nov 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion paddle/utils/Util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pid_t getTID() {
#endif
pid_t tid = syscall(__NR_gettid);
#endif
CHECK_NE(tid, -1);
CHECK_NE((int)tid, -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

此处对常数加类型控制，会不会更好？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@backyes 但是这里tid变量 在不同操作系统类型不一样。

Copy link
Contributor

Choose a reason for hiding this comment

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

@gangliao 如果这个问题很严重，建议还是加一层抽象，否则换个平台你这种强制int 可能还会有问题。
如果这个BUG比较紧急，可以考虑先ci解决临时问题，长期还是要系统解决下这类问题，可以考虑加到下下个milestone

Copy link
Collaborator

Choose a reason for hiding this comment

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

并不会很严重吧？其实对于size_t也一样。有的平台是 uint32_t，有的平台是 uint64_t。直接比较就会有问题。需要在比较前强制转型。

这种问题C/C++里面挺常见的。

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gangliao 因为这里 tid的类型是 uint64_t，所以应该改成 CHECK_NE(uint64_t(-1), tid)。

@reyoung @backyes 这里就是应该有type cast，具体原因请参见这里的解释： https://google.github.io/styleguide/cppguide.html#Integer_Types

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的讨论是为了让这个pr能build过呢？还是让mac port能工作呢？还是也考虑将来的windows port呢？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wangkuiyi 比较麻烦的是 各大平台获得thread id的方式不大相同。 如果为了统一可以考虑直接改了c++11接口的方式

Copy link
Collaborator

@wangkuiyi wangkuiyi Nov 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@gangliao 是的。我理解真要让代码可移植（portable），需要整理代码，使其符合很多编码规范（patterns），比如这里列出了一些。我注意到我们目前的代码还有很多地方不符合这些规范的。如果我们现在开始port，可能要花很多精力在不断涌现的各种不portable的具体例子里。

我想是不是我们先不要支持mac和windows了，先把linux/docker搞定——这样我们可以集中精力先把tutorials，和清华北大的课程（windows和mac上都可以跑docker），以及在各大公有云上跑起来这些事儿先做好，让paddle能被更多的人先用起来，别让做课程以及做运营的同事们等着。随后，我们再来全面整顿代码，使其具备可移植性，然后支持 windows, mac, ios, arm 等平台？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wangkuiyi 好的，明白了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gangliao 我支持提一个建议。我们一起考虑一下，比如周一再定不迟。:-)

如果真的决定如此，我们恐怕得从目前的Build and Install页面里把mac部分的内容删掉呢。

return tid;
}

Expand Down