Skip to content

Commit

Permalink
Removing bad suggestion. In theory, we could change this signature to…
Browse files Browse the repository at this point in the history 
… take a const reference to a string, but for now, I'm reverting it.
  • Loading branch information
@jonoomph
jonoomph committed Jun 5, 2020
1 parent 51fe854 commit 7b94ac8
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion include/FFmpegWriter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ namespace openshot {

/// @brief Constructor for FFmpegWriter. Throws one of the following exceptions.
/// @param path The file path of the video file you want to open and read
FFmpegWriter(const std::string path);
FFmpegWriter(std::string path);

This comment has been minimized.

Sign in to view

Copy link
@ferdnyc

ferdnyc Jun 6, 2020 •
edited

Contributor

I think the issue here was that FFmpegWriter should take path by const reference — not a const path. IOW, the recommendation was to declare this as:

FFmpegWriter(const std::string& path);

And then it would be defined the same way in the .cpp:

FFmpegWriter::FFmpegWriter(const std::string& path) :
	path(path), fmt(NULL), oc(NULL), audio_st(NULL), video_st(NULL), samples(NULL),
	audio_outbuf(NULL), audio_outbuf_size(0), audio_input_frame_size(0), audio_input_position(0)...

That should work transparently (but more efficiently) in all situations. It just tells the compiler that it doesn't have to copy path before passing it to the constructor — it can just pass a reference, because the original won't be modified. (The constructor will make a copy anyway, when it stores the referenced string to the member variable path in the initializer list.)

If you pass a const string, then you have to cast away the const-ness before you can store it in a non-const member variable. But a const reference shouldn't have that issue.

This comment has been minimized.

Sign in to view

Copy link
@ferdnyc

ferdnyc Jun 6, 2020

Contributor

I actually have a WIP branch somewhere where I've been fixing up our data-passing across the board, for all of the complex types. (Including our own, when we pass them around by value a lot. Anything that takes openshot::Keyframe, openshot::Point, etc. arguments can benefit the same way.)

/// Close the writer
void Close();
Expand Down
2 changes: 1 addition & 1 deletion src/FFmpegWriter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ static int set_hwframe_ctx(AVCodecContext *ctx, AVBufferRef *hw_device_ctx, int6
}
#endif // HAVE_HW_ACCEL

FFmpegWriter::FFmpegWriter(const std::string path) :
FFmpegWriter::FFmpegWriter(std::string path) :
path(path), fmt(NULL), oc(NULL), audio_st(NULL), video_st(NULL), samples(NULL),
audio_outbuf(NULL), audio_outbuf_size(0), audio_input_frame_size(0), audio_input_position(0),
initial_audio_input_frame_size(0), img_convert_ctx(NULL), cache_size(8), num_of_rescalers(32),
Expand Down

0 comments on commit 7b94ac8

Please sign in to comment.