|
|
Patch Set 1 #Patch Set 2 : Addressed comments from Vlad and Chandler. #Patch Set 3 : Adds description on building gtest on one's own. #Total messages: 9
|
Vlad
I like it, with a small correction. Chandler, do you have anything to say? http://codereview.appspot.com/8864/diff/1/3 ...
|
17 years, 1 month ago (2008年11月15日 02:10:20 UTC) #1 |
I like it, with a small correction. Chandler, do you have anything to say? http://codereview.appspot.com/8864/diff/1/3 File README (right): http://codereview.appspot.com/8864/diff/1/3#newcode23 Line 23: also make our best effort to support other platforms (e.g. Solaris and In this case, even with their best effort, the amount of support the core members of the team are able to provide is uncomfortably close to zero. Maybe later when will have our cross-platform test infrastructure done, we'll set up builds and tests of Google Test on all those strange and beautiful OSes, but for now I propose to change the rest of this paragraph to he following: We rely on our external contributors to support Google Test on other platforms (such as Solaris and IBM z/OS) because core members of the Google Test team have no access to them. Bug reports and (especially!) patches for those platforms are very welcome at googletestframework@googlegroups.com.
http://codereview.appspot.com/8864/diff/1/3 File README (right): http://codereview.appspot.com/8864/diff/1/3#newcode19 Line 19: This blank line is inconsistent with this file's formatting. http://codereview.appspot.com/8864/diff/1/3#newcode23 Line 23: also make our best effort to support other platforms (e.g. Solaris and On 2008年11月15日 02:10:20, Vlad wrote: > In this case, even with their best effort, the amount of support the core > members of the team are able to provide is uncomfortably close to zero. Maybe > later when will have our cross-platform test infrastructure done, we'll set up > builds and tests of Google Test on all those strange and beautiful OSes, but for > now I propose to change the rest of this paragraph to he following: > > We rely on our external contributors to support Google Test on other platforms > (such as Solaris and IBM z/OS) because core members of the Google Test team have > no access to them. Bug reports and (especially!) patches for those platforms are > very welcome at googletestframework@googlegroups.com. I think "best effort" and Zhanyong's disclaimer of access restrictions is pretty good. If someone says "the compiler on my weird machine says '...', what do I do?" we can certainly look at the code, and provide some guesses as to how they should debug. We also conform to standards that should be supported across tool chains. Sounds like "best effort" to me. ;] http://codereview.appspot.com/8864/diff/1/3#newcode25 Line 25: have no access to them, some versions of Google Test may not work well "some versions" is confusing here to me when reading as Joe the Coder. How about "Google Test may have outstanding issues on these platforms"? That makes it clear that we're hoping to address them, but haven't gotten there yet, and leads nicely into the next sentence. http://codereview.appspot.com/8864/diff/1/2 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/8864/diff/1/2#newcode79 Line 79: // most stable support. Since core members of the Google Test project Rather than the last 5 lines of this, why not refer back to the README after clarifying the four which have strong support?
I've uploaded a new snapshot. Another look? Thanks. http://codereview.appspot.com/8864/diff/1/3 File README (right): http://codereview.appspot.com/8864/diff/1/3#newcode19 Line 19: On 2008年11月15日 02:17:17, chandlerc wrote: > This blank line is inconsistent with this file's formatting. Done. http://codereview.appspot.com/8864/diff/1/3#newcode23 Line 23: also make our best effort to support other platforms (e.g. Solaris and On 2008年11月15日 02:10:20, Vlad wrote: > In this case, even with their best effort, the amount of support the core > members of the team are able to provide is uncomfortably close to zero. Maybe > later when will have our cross-platform test infrastructure done, we'll set up > builds and tests of Google Test on all those strange and beautiful OSes, but for > now I propose to change the rest of this paragraph to he following: > > We rely on our external contributors to support Google Test on other platforms > (such as Solaris and IBM z/OS) because core members of the Google Test team have > no access to them. Bug reports and (especially!) patches for those platforms are > very welcome at googletestframework@googlegroups.com. Obviously I'm biased, but here's the thinking: - "make our best effort" != "guaranteed to work", so we are not over-promosing to say that we'll make our beest effort. - Simply saying that we rely on external contributors to support other platforms can be misleading, as it gives people the impression that we don't care. http://codereview.appspot.com/8864/diff/1/3#newcode25 Line 25: have no access to them, some versions of Google Test may not work well On 2008年11月15日 02:17:17, chandlerc wrote: > "some versions" is confusing here to me when reading as Joe the Coder. How about > "Google Test may have outstanding issues on these platforms"? That makes it > clear that we're hoping to address them, but haven't gotten there yet, and leads > nicely into the next sentence. Done. http://codereview.appspot.com/8864/diff/1/2 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/8864/diff/1/2#newcode79 Line 79: // most stable support. Since core members of the Google Test project On 2008年11月15日 02:17:17, chandlerc wrote: > Rather than the last 5 lines of this, why not refer back to the README after > clarifying the four which have strong support? My rationale is that it's better user experience if the user doesn't have to open another file and find the right paragraph in it. Yes, there's some duplication of words, but I think it's not significant and well worth it. Do you still want me to change this?
I added a section on using one's own build system to build gtest. Please take a look at the latest snapshot. Thanks!
http://codereview.appspot.com/8864/diff/604/405 File README (right): http://codereview.appspot.com/8864/diff/604/405#newcode223 Line 223: You should also let our users know that they can use SCons to build the project and their own tests. We already have the SConscript; the users just have to provide their own SConstruct file.
http://codereview.appspot.com/8864/diff/604/405 File README (right): http://codereview.appspot.com/8864/diff/604/405#newcode223 Line 223: On 2008年11月17日 21:42:07, Vlad wrote: > You should also let our users know that they can use SCons to build the project > and their own tests. We already have the SConscript; the users just have to > provide their own SConstruct file. Good idea. Could you let me know which platforms our SConscript supports?
http://codereview.appspot.com/8864/diff/604/405 File README (right): http://codereview.appspot.com/8864/diff/604/405#newcode223 Line 223: On 2008年11月17日 21:54:00, Zhanyong wrote: > On 2008年11月17日 21:42:07, Vlad wrote: > > You should also let our users know that they can use SCons to build the > project > > and their own tests. We already have the SConscript; the users just have to > > provide their own SConstruct file. > > Good idea. Could you let me know which platforms our SConscript supports? > Hrm. Come to think of it, the HEAD version in svn has Windows specific features. The patch at http://codereview.appspot.com/8654/diff/1/23 pulls in changes that make it cross platform. So it makes sense to mention SCons only after we commit that patch -- or a separate one with just that file (amended, of course). I withdraw my comment until then.